Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding JSDoc to Paste Components #3902

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

cogwizzle
Copy link
Collaborator

The purpose of this Pull Request is to add some JSDoc to the Account Switcher, Alert, and Alert Dialog components.

There are a few minor refactors that accompany this Pull Request that should not impact the functionality of the component. These changes are purely focused on reducing boilerplate or reducing the complexity of the syntax of the components. All of these changes are confined in the AccountSwitcher, Alert, or Alert Dialog components or their supporting components.

Additional JSDoc changes will be coming however, new changes will be broken out into individual component Pull Request to reduce the load on the reviewing engineer.

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

Copy link

stackblitz bot commented May 10, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

changeset-bot bot commented May 10, 2024

⚠️ No Changeset found

Latest commit: c8fc1c1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented May 10, 2024

@cogwizzle is attempting to deploy a commit to the Twilio Team on Vercel.

A member of the Team first needs to authorize it.

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label May 10, 2024
Copy link

nx-cloud bot commented May 10, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit c8fc1c1. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

Comment on lines +15 to +18
/**
* Animation states for the Alert Dialog.
*/
const AnimationStates = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we were attempting to avoid using the ReactSpring types here. I replaced the function with a hard coded object so that we weren't creating new objects each time this component ran. Further down in the component I converted from the SpringValue type to the native type so we don't have to use any.

Comment on lines 114 to 120
const normalizeStyles = (styles: NormalizeStylesArg): NormalizeStylesReturn => {
return {
...styles,
opacity: styles.opacity.get(),
transform: styles.transform.get()
};
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function converts our SpringValue to the native generic value of the SpringValue. This normalizes the styles and allows us to avoid the any above.

Comment on lines -116 to -118
return (
<>
{transitions(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need a fragment wrapping this so I removed some of the code wrapping this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review this file carefully to ensure no regressions were introduced please.

onDismissLabel: string;
/** Property to disable the confirm button. _Has no effect if destructive is not true._ */
onConfirmDisabled?: boolean;
Copy link
Collaborator Author

@cogwizzle cogwizzle May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made this comment in the discussions, but I think we should rename this internal components property to isConfirmDisabled to better communicate that it is a boolean. I would not make this change as part of this PR.

Comment on lines 110 to 128
const renderAlertIcon = (variant: AlertVariants, element: string, title: string): React.ReactElement => {
interface AlertIconProps {
variant: AlertVariants;
element: string;
title: string;
}

/**
* Component to display the appropriate alert icon.
*/
const AlertIcon: React.FC<AlertIconProps> = ({
variant,
element,
title,
}): React.ReactElement => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored this to be a component because it is a function that takes in args and returns JSX. It effectively had a signature similar to a React component.

Copy link

codesandbox-ci bot commented May 10, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit c8fc1c1:

Sandbox Source
@twilio-paste/nextjs-template Configuration
@twilio-paste/token-contrast-checker Configuration

Copy link
Collaborator

@nkrantz nkrantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @cogwizzle - thanks for this contribution!

I'd like to think a bit more about these changes. I definitely see the value in having the JSDoc comments on the consumer-facing components that include the package description and the documentation link, but I'm a little concerned about the maintenance there. It's not the most common, but sometimes we do change package descriptions, and there are already multiple places we need to maintain them (the package's package.json and all 3 files within the packages/paste-website/src/pages/components/* directory). Having an additional place in the component file itself seems like too much upkeep that's prone to human error. I wonder if we could instead pull the description from the package.json to create the comments. It would be helpful to similarly import or dynamically create the documentation url so that doesn't add additional labor to the creation of new components, but I'd have to do more thinking as to whether that's possible as some of the docs urls aren't exactly standard (e.g. Sidebar).

In terms of the non-consumer-facing components, could you explain what the value-add is for adding JSDoc comments? Such as AlertDialogBody, AlertDialogFooterProps props, AlertIcon, etc. If it's for contributors, I'm not completely convinced it outweighs the clutter of having them in the docs and effort of creating and maintaining them. I'm also not totally convinced that consumer-facing components that exist within a single component (e.g. Account Switcher's AccountSwitcherBadge component) need to have the comments. It should be clear from the documentation what these compositional components are for and creating descriptions for each one for the comments will similarly take additional time and effort. Happy to be swayed otherwise (on any of this).

If we decide to move forward with these JSDoc comments (or some portion of them, even if only on consumer-facing components), it would be great to get them all out across the board in a relatively timely manner, so that the change happens across not too many upgrades for consumers. In the past when we've done core-wide changes, we've split them up alphabetically into batches (a-c, d-h, i-p, etc) so just sharing that as an option.

Either way, thanks again for this improvement and for all of your recent improvements. It's great to have consumers of Paste contributing and you bring a really valuable perspective to the project! Sadly the team is operating at extremely low capacity right now so we can't get them reviewed and merged as quickly as we'd like, but we appreciate your patience.

@cogwizzle
Copy link
Collaborator Author

cogwizzle commented May 24, 2024

Hey @cogwizzle - thanks for this contribution!

I'd like to think a bit more about these changes. I definitely see the value in having the JSDoc comments on the consumer-facing components that include the package description and the documentation link, but I'm a little concerned about the maintenance there. It's not the most common, but sometimes we do change package descriptions, and there are already multiple places we need to maintain them (the package's package.json and all 3 files within the packages/paste-website/src/pages/components/* directory). Having an additional place in the component file itself seems like too much upkeep that's prone to human error. I wonder if we could instead pull the description from the package.json to create the comments. It would be helpful to similarly import or dynamically create the documentation url so that doesn't add additional labor to the creation of new components, but I'd have to do more thinking as to whether that's possible as some of the docs urls aren't exactly standard (e.g. Sidebar).

In terms of the non-consumer-facing components, could you explain what the value-add is for adding JSDoc comments? Such as AlertDialogBody, AlertDialogFooterProps props, AlertIcon, etc. If it's for contributors, I'm not completely convinced it outweighs the clutter of having them in the docs and effort of creating and maintaining them. I'm also not totally convinced that consumer-facing components that exist within a single component (e.g. Account Switcher's AccountSwitcherBadge component) need to have the comments. It should be clear from the documentation what these compositional components are for and creating descriptions for each one for the comments will similarly take additional time and effort. Happy to be swayed otherwise (on any of this).

If we decide to move forward with these JSDoc comments (or some portion of them, even if only on consumer-facing components), it would be great to get them all out across the board in a relatively timely manner, so that the change happens across not too many upgrades for consumers. In the past when we've done core-wide changes, we've split them up alphabetically into batches (a-c, d-h, i-p, etc) so just sharing that as an option.

Either way, thanks again for this improvement and for all of your recent improvements. It's great to have consumers of Paste contributing and you bring a really valuable perspective to the project! Sadly the team is operating at extremely low capacity right now so we can't get them reviewed and merged as quickly as we'd like, but we appreciate your patience.

Thanks so much for reviewing this PR. I agree that the maintenance upkeep might too high. I would love to find a way to pull the documentation from an external source or to find a way to keep them in sync.

As far as the value proposition it is extremely helpful to understand the use case for a component while using a library. JSDoc is just the way that I am aware of to make this happen. Perhaps there is another way to get the Language Server to add context. A good example of where a consumer of paste would need context is for things like button accessibility guidance. Without this great guidance an engineer could consume this in the wrong way. I doubt every engineer hits the Paste site every time they want to consume a component. It is unlikely the consumer of the library would see this great advice if they did not have the JSDoc that we previously added for this. There are also other contextually important information such as understanding when to use an Alert vs when to use a Callout. The description information in the documentation can help here.

This contextual information is relevant when consumers use libraries and they can use the TypeScript language server to give details about the element. Another tangental thing we could do with JSDoc is use it for typing if we wanted to drop TypeScript, but that doesn't seem relevant in this project.

Here is an example of storybook providing its users contextual information in TS Server. I'm not sure if they're using JSDoc or some other method to accomplish this.
image

The short version is I want all of the great guidance that exist in the Paste docs to be quickly accessible from the consuming engineers.

PS: For the non public stuff I just added JSDoc for consistency. I wasn't 100% sure that they wouldn't be used outside of the Paste project.

@cogwizzle
Copy link
Collaborator Author

I wonder if we could take the opposite approach to solving the TSDoc problem. I wonder if we could extract a basic description from the code files and then have elaborative details. This is a tough problem to solve either way.

@cogwizzle
Copy link
Collaborator Author

I wonder if we could take the opposite approach to solving the TSDoc problem. I wonder if we could extract a basic description from the code files and then have elaborative details. This is a tough problem to solve either way.

Apparently it is possible https://tsdoc.org/#why-do-we-need-tsdoc. Perhaps it is worth digging into?

@serifluous serifluous added the Contribution This is a contribution label Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contribution This is a contribution size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants