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

Components: enhancements to TypeScript migration guidelines #41669

Merged
merged 6 commits into from
Jun 14, 2022

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Jun 10, 2022

What?

  • Add link to TypeScript migration guide in TOC
  • Add a point about props destructuring
  • Expand point about WordPressComponentProps

Why?

Iteratively improve the TypeScript migration guidelines as we gather insights from the initial work on the matter

@ciampo ciampo added [Type] Developer Documentation Documentation for developers [Package] Components /packages/components labels Jun 10, 2022
@ciampo ciampo requested a review from mirka June 10, 2022 17:57
@ciampo ciampo added this to In progress (owned) ⏳ in WordPress Components via automation Jun 10, 2022
@ciampo ciampo self-assigned this Jun 10, 2022
@ciampo ciampo requested a review from ajitbohra as a code owner June 10, 2022 17:57
}
```

3. Add the folders to the `tsconfig.json` file.
Copy link
Contributor Author

@ciampo ciampo Jun 10, 2022

Choose a reason for hiding this comment

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

Points 3, 4 and 5 here are unchanged apart from their index, that shiftet by 1 after adding a new point (now no. 2)

Comment on lines +496 to +501
// Before:
function MyComponent( { myProp1, myProp2, ...restProps } ) { /* ... */ }

// After:
function MyComponent( props ) {
const { myProp1, myProp2, ...restProps } = props;
Copy link
Member

Choose a reason for hiding this comment

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

Good clarification with the example 👍

import type { ComponentOwnProps } from './types';

function UnconnectedMyComponent(
// The resulting type will include:
Copy link
Member

Choose a reason for hiding this comment

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

Should we add the stuff about the ref?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed some changes in cb34385, let me know what you think!

Main changes:

  • moved down the point re. making sure that there's a name export
  • added a new point re. ref forwarding (either via forwardRef or via contextConnect)
  • updated index of remaining points as needed

@walbo
Copy link
Member

walbo commented Jun 13, 2022

Maybe update the section with @example as well? The code example doesn't appear in the stories Docs tab if you add @example before the code. See #40915 (comment)

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

🚀

packages/components/CONTRIBUTING.md Show resolved Hide resolved
@mirka
Copy link
Member

mirka commented Jun 14, 2022

Maybe update the section with @example as well? The code example doesn't appear in the stories Docs tab if you add @example before the code. See #40915 (comment)

Good point. I was kind of on the fence about showing the @example snippets in the Storybook — on one hand they're a bit redundant given the actual story code snippets, but on the other hand the @example snippets are more complete with the import and useState statements. I don't feel strongly either way though. Do y'all have a preference?

@ciampo
Copy link
Contributor Author

ciampo commented Jun 14, 2022

I've updated the point about the @example keyword in ff88bb5 , discouraging its usage.

Do y'all have a preference?

Let's go with showing the extra snippet in the StoryBook docs, for now. As you say, these snippets can offer a more complete example (with imports and useStates).

Also, sometimes the dynamic code snippet from StoryBook are not as helpful (e.g. sometimes they show function noRefCheck(){} for callback props), so I believe it's probably a good thing to have an extra code snippet.

@ciampo ciampo force-pushed the feat/components-typescript-migration-improvements branch from fe0646f to da279b7 Compare June 14, 2022 11:32
@ciampo ciampo merged commit 6bf86eb into trunk Jun 14, 2022
WordPress Components automation moved this from In progress (owned) ⏳ to Done 🎉 Jun 14, 2022
@ciampo ciampo deleted the feat/components-typescript-migration-improvements branch June 14, 2022 12:51
@github-actions github-actions bot added this to the Gutenberg 13.5 milestone Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Developer Documentation Documentation for developers
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants