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.

Sign up for GitHub

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

Jump to bottom

Menu: auto-generate README #68249

Merged
merged 7 commits into from
Jan 9, 2025
Merged

Menu: auto-generate README #68249

merged 7 commits into from
Jan 9, 2025

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Dec 23, 2024 •
edited
Loading

What?

Convert the Menu README to an auto-generated one.

Supplementary information that was in the existing README is moved to other appropriate locations (JSDocs, new "Best Practices" Storybook page).

Why?

To decrease maintenance cost and consolidate the canonical docs for our audience.

Testing Instructions

  • In your IDE, check imports of Menu.Item for example, and see that the JSDoc includes the subcomponent description.
  • npm run docs:components to regenerate READMEs.
  • See Storybook docs for Menu.

@ciampo ciampo force-pushed the feat/menu-generate-readme branch 2 times, most recently from 93215f7 to 323f875 Compare December 23, 2024 16:14
ciampo
ciampo commented Dec 23, 2024
View reviewed changes
packages/components/src/menu/stories/index.story.tsx

const meta: Meta< typeof Menu > = {
id: 'components-experimental-menu',
id: 'components-menu',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was necessary because the generated README links to ?path=/docs/components-menu--docs. I've updated storybook/manager-head.html to add a redirect from the old experimental URL.

ciampo
ciampo commented Dec 23, 2024
View reviewed changes
packages/components/src/menu/index.tsx
* It can optionally contain one instance of the `Menu.ItemLabel` component
* and one instance of the `Menu.ItemHelpText` component.
*/
Item: Object.assign( Item, {
Copy link
Contributor Author

@ciampo ciampo Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

With the previous subcomponent named export scheme (ie. MenuItem being assigned to the Item property of the Menu object), the README generator could not pick up the JSDoc correctly,

I managed to fix it by changing the internal naming convention of all subcomponent by removing the Menu prefix: for example, MenuItem got renamed to Item. A lot of the code changes in this file are related to this refactor and were applied as a single commit. You can review the rest of the changes commit by commit to remove the noise.

ciampo
ciampo commented Dec 23, 2024
View reviewed changes
packages/components/src/menu/README.md
states and behaviors:
- The `portal` and `preventBodyScroll` props are set to `true`. They can
still be manually set to `false`.
- When the dialog is open, element tree outside it will be inert.
Copy link
Contributor Author

@ciampo ciampo Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@mirka here's another instance of the problem you were mentioning with list being merged when rendered to HTML ( #68209 (comment) )

@ciampo ciampo self-assigned this Dec 23, 2024
@ciampo ciampo requested a review from a team December 23, 2024 16:26
@ciampo ciampo added [Type] Developer Documentation Documentation for developers [Package] Components /packages/components labels Dec 23, 2024
@ciampo ciampo marked this pull request as ready for review December 23, 2024 16:26
@ciampo ciampo requested a review from ajitbohra as a code owner December 23, 2024 16:26
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 23, 2024 •
edited
Loading

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@ciampo ciampo force-pushed the feat/menu-generate-readme branch from 323f875 to 364338a Compare December 23, 2024 16:54
@ciampo ciampo mentioned this pull request Dec 23, 2024
Open
58 tasks
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 23, 2024 •
edited
Loading

Flaky tests detected in c5d0b26.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12692406417
📝 Reported issues:

tyxla
tyxla reviewed Dec 30, 2024
View reviewed changes
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

Needs a rebase and rerun to get the newest improvements.

packages/components/src/menu/README.md Show resolved Hide resolved
@ciampo ciampo force-pushed the feat/menu-generate-readme branch from 364338a to 510391e Compare January 8, 2025 17:51
@ciampo
Copy link
Contributor Author

ciampo commented Jan 8, 2025

@tyxla rebased and re-generated README. Could you give a final round of review?

@ciampo ciampo requested a review from tyxla January 8, 2025 17:52
tyxla
tyxla approved these changes Jan 9, 2025
View reviewed changes
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

LGTM 👍

packages/components/src/menu/context.tsx Show resolved Hide resolved
packages/components/src/menu/index.tsx Outdated Show resolved Hide resolved
@ciampo ciampo force-pushed the feat/menu-generate-readme branch from 510391e to c5d0b26 Compare January 9, 2025 14:53
@ciampo ciampo enabled auto-merge (squash) January 9, 2025 14:54
@ciampo ciampo merged commit ddfc025 into trunk Jan 9, 2025
64 of 66 checks passed
@ciampo ciampo deleted the feat/menu-generate-readme branch January 9, 2025 16:17
@github-actions github-actions bot added this to the Gutenberg 20.1 milestone Jan 9, 2025
Gulamdastgir-Momin pushed a commit to Gulamdastgir-Momin/gutenberg that referenced this pull request Jan 23, 2025
@ciampo @mirka
@tyxla @Gulamdastgir-Momin
Menu: auto-generate README (WordPress#68249)
2ca41d8 
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mirka mirka mirka left review comments

@tyxla tyxla tyxla approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

Assignees

@ciampo ciampo

Labels
[Package] Components /packages/components [Type] Developer Documentation Documentation for developers
Projects
None yet
Milestone
Gutenberg 20.1
Development

Successfully merging this pull request may close these issues.
3 participants
@ciampo @mirka @tyxla