Button: Revise documentation #66617
Merged
Button: Revise documentation #66617
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mirka
added
[Type] Developer Documentation
|
Size Change: 0 B Total Size: 1.83 MB ℹ️ View Unchanged
|
|
Flaky tests detected in ec69ef6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11839917233
|
mirka
commented
Oct 31, 2024
There was a problem hiding this comment.
How about we write the supplementary docs in a file like this? I'm having second thoughts about putting all the documentation into the main Docs page, because it can already be quite long with the props table and the stories.
mirka
commented
Oct 31, 2024
There was a problem hiding this comment.
This is now auto-generated using the npm run docs:components command.
mirka
commented
Oct 31, 2024
Comment on lines
+117
to
+120
| * 1. `'primary'` (the primary button styles) | ||
| * 2. `'secondary'` (the default button styles) | ||
| * 3. `'tertiary'` (the text-based button styles) | ||
| * 4. `'link'` (the link button styles) |
There was a problem hiding this comment.
These descriptions need to be improved (see size prop description for example). It also fails to capture that there is a default (variant={ undefined }) variant.
5 tasks
- added content - added embeds
Removed sample Figma embed link
- Fixed Figma links
mirka
commented
Nov 15, 2024
| # Button | ||
| Buttons allow users to take actions and make selections with a single click or tap. | ||
|
|
||
| <FigmaEmbed url="https://www.figma.com/design/V7NfxdP0ybEMXf01WyWGKB/Storybook-documentation-Figma-assets?node-id=5-280&node-type=frame&t=JLKM1grrtWleRkFR-11" title="kitchen-sink-button" height="400" /> |
There was a problem hiding this comment.
@auareyou I'm wondering if we can set up a better frame size for the Figma embeds, so the content isn't needlessly scaled down when rendered in the iframe. The max content width for the Storybook doc page is 1000px, while the Figma frames (for example this) are 1280px wide. Does the initial zoom factor become closer to 100% when the Figma frame widths are reduced? (I would love to just set a fixed 100% zoom factor from the code side, but I don't think there's a way to do that.)
There was a problem hiding this comment.
Yes, @mirka, we can definitely change the height. Are you suggesting changing the width to 1000px in the images and then whatever height is proportional?
There was a problem hiding this comment.
Are you suggesting changing the width to 1000px in the images and then whatever height is proportional?
Right. But I don't know whether the height proportionality affects the initial zoom factor, and I don't have edit access for Figma, so could you experiment a bit to see if there is a better size setup? It's ok if nothing works, but just wanted to check if there's an easy way to make it closer to 100%.
There was a problem hiding this comment.
I've resized them to 1000px by 300px. Let me know if it doesn't work.
There was a problem hiding this comment.
Did you experiment with a couple values? It's a lot closer to 100%, but still slightly off (the buttons are 36px high instead of 40px), no? Seems like something we can get right once, and reuse those metrics for everything.
auareyou
approved these changes
Nov 21, 2024
| # Button | ||
| Buttons allow users to take actions and make selections with a single click or tap. | ||
|
|
||
| <FigmaEmbed url="https://www.figma.com/design/V7NfxdP0ybEMXf01WyWGKB/Storybook-documentation-Figma-assets?node-id=5-280&node-type=frame&t=JLKM1grrtWleRkFR-11" title="kitchen-sink-button" height="400" /> |
There was a problem hiding this comment.
I've resized them to 1000px by 300px. Let me know if it doesn't work.
mirka
commented
Nov 21, 2024
| # Button | ||
| Buttons allow users to take actions and make selections with a single click or tap. | ||
|
|
||
| <FigmaEmbed url="https://www.figma.com/design/V7NfxdP0ybEMXf01WyWGKB/Storybook-documentation-Figma-assets?node-id=5-280&node-type=frame&t=JLKM1grrtWleRkFR-11" title="Overview of button variants" height="400" /> |
There was a problem hiding this comment.
@auareyou I went in and fixed them all in this one, but FYI the title attribute is an accessible name, not an id. So it needs to be meaningful text.
Comment on lines
+68
to
+72
| /** | ||
| * Primary buttons stand out with bold color fills, making them distinct | ||
| * from the background. Since they naturally draw attention, each layout should contain | ||
| * only one primary button to guide users toward the most important action. | ||
| */ |
There was a problem hiding this comment.
I am reevaluating this PR from a user perspective, and I realized that we're probably not adding much value by showing a separate page with variants, including a full specimen of hover/active/disabled states. Also, some of the states are already wrong (i.e. not the same as in shipping code). For example, look at all the Link variants — they're all wrong. @auareyou Can you troubleshoot this? Are we using the Figma library components incorrectly in some way?
What I see in the Figma embed
Actual Link variant (default)
We already show an interactive list of all the variants in the normal story docs, so I suggest we just move the copy there. That's where all this kind of basic, important information should live. What do you think?
The Best Practices page is probably a good home for more supplementary information, like patterns/examples that don't quite fit in the normal story docs.
There was a problem hiding this comment.
Are we using the Figma library components incorrectly in some way?
There's an issue in Figma where replacing underlined text layer values removes the underline 🙃 I fixed it.
I am reevaluating this PR from a user perspective, and I realized that we're probably not adding much value by showing a separate page with variants, including a full specimen of hover/active/disabled states.
I tend to agree with this though. The value doesn't quite reflect the effort, imo.
There was a problem hiding this comment.
I moved all the variant descriptions to the main stories 👍
Figma embeds should be a last resort, given the slow load times and maintenance cost.
|
Should be fixed: 
|
Yep, they're loading perfectly fine now, thanks ✅ |
|
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. |
|
@ciampo Although we need to do some re-workshopping on the supplementary doc template, I think we are ready to merge this iteration. The only irreversible bit is if we care about the permalink for the "Best Practices" page, which could possibly change. (I think we don't have to care about that at this point yet.) I'd appreciate a quick thumbs up from you 🙏 |
ciampo
reviewed
Dec 10, 2024
There was a problem hiding this comment.
Chat notes:
- Add TOC to Storybook generated DOCS page, linking to props and best practices
- Add "Accessibility" section
- Add a "Related components" section at the bottom
- Add section on how to write component's documentation in the contributing guidelines
- [optional] add linter to enforce some of the guidelines above
yogeshbhutkar
pushed a commit
to yogeshbhutkar/gutenberg
that referenced
this pull request
Dec 18, 2024
* Button: Auto-generate README * Add additional mdx file * Fixup to prevent merging consecutive lists * Add FigmaEmbed component * Update best-practices.mdx - added content - added embeds * Update best-practices.mdx Removed sample Figma embed link * Update best-practices.mdx - Fixed Figma links * Fixup * Fix iframe titles * Replace sizes section * Move variant description to story description? * Move variant descriptions to main docs --------- Co-authored-by: mirka <[email protected]> Co-authored-by: auareyou <[email protected]> Co-authored-by: jameskoster <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: tyxla <[email protected]> Co-authored-by: ciampo <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Moves the supplementary Button docs to the Storybook, and expands on the content.
Why?
As we evolve the design system, we need better documentation.
How?
best-practices— not sure this is the best name, could be better.)FigmaEmbedcomponent for easy embedding of Figma links.Testing Instructions
Run
npm run storybook:dev. You should see a new Best Practices page under the folder for Button. Review the content.Screenshots or screencast