-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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: add "Naming conventions" section #63714
Components: add "Naming conventions" section #63714
Conversation
@WordPress/gutenberg-components I took an initial stab at updating the contributing guidelines. Also cc @diegohaz in case you have any feedback. |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
- [README example](#README-example) | ||
- [Folder structure](#folder-structure) | ||
- [Component versioning](#component-versioning) | ||
- [Introducing new components](#introducing-new-components) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My IDE insists on auto-formatting this file. I've put all the auto-formatting changes in one single commit, but if you think they're too distracting / they don't belong to this PR I'm happy to remove and potentially apply them in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When adding new components or new props to existing components, it's recommended to create a [private version](/packages/private-apis/README.md)) of the component until the changes are stable enough to be exposed as part of the public API. | ||
|
||
### Learn more | ||
|
||
- [How to preserve backward compatibility for a React Component](/docs/contributors/code/backward-compatibility.md#how-to-preserve-backward-compatibility-for-a-react-component) | ||
- [Experimental and Unstable APIs](/docs/contributors/code/coding-guidelines.md#experimental-and-unstable-apis) | ||
- [Deprecating styles](#deprecating-styles) | ||
- [How to preserve backward compatibility for a React Component](/docs/contributors/code/backward-compatibility.md#how-to-preserve-backward-compatibility-for-a-react-component) | ||
- [Experimental and Unstable APIs](/docs/contributors/code/coding-guidelines.md#legacy-experimental-apis-plugin-only-apis-and-private-apis) | ||
- [Deprecating styles](#deprecating-styles) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this section to reflect the new "private APIs" policy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more or less looking good, but still has some debatable elements (the Component.Root
dot notation mostly) that still needs a stronger consensus in the #63242 discussion at this time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far, though it seems like we're switching to an "overloaded" API?
@DaniGuardiola and I were discussing some potential TypeScript problems with overloading when It's a bit finicky where you need to place your JSDocs and |
b26f91e
to
bdfe95c
Compare
I've updated the PR suggesting the "overloaded" naming convention and refreshing the code examples. Could y'all take a new look? (as I'm going to be AFK for a week, feel free to push updates and/or merge in case in my absence) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great 👍
I've left some minor things mostly, I'll let @mirka and @DaniGuardiola take a look and share their feedback before we ship it.
packages/components/CONTRIBUTING.md
Outdated
const Context = createContext(); | ||
|
||
/** The top-level component's JSDoc. */ | ||
export const Component = Object.assign( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be a good opportunity to explain the purpose behind using Object.assign()
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and it might be useful to state what exactly we need to look for:
- When consuming the component/subcomponent, the JSDocs must appear correctly in IntelliSense.
- The main component JSDoc must appear in the Storybook docs page.
- The component/subcomponent prop types must appear correctly in the Storybook props table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a couple of paragraphs explaining requirements and related recommended best practices: 432635b
This way the only comments in the code are related to the JSDocs
- [README example](#README-example) | ||
- [Folder structure](#folder-structure) | ||
- [Component versioning](#component-versioning) | ||
- [Introducing new components](#introducing-new-components) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the update! I had a few notes on top of @tyxla's review, but otherwise this looks great 👍
…monolithic components
737dbb6
to
432635b
Compare
All feedback should be addressed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go from my perspective. Thanks @ciampo 🚀
Flaky tests detected in cb6485c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10299502863
|
What?
Closes #63242
Tweak the contributing guidelines, updating a few out of date parts and adding a new "Naming Conventions" section based on the conversation happening in #63242
Why?
As we introduce more compound components in the package, we need a recommended naming conventions for both monolithic and compound components going forward.