-
Notifications
You must be signed in to change notification settings - Fork 10
Storybook docs formatting update #2692
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
base: main
Are you sure you want to change the base?
Conversation
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.
Just reviewed the rendered button story and its content, not the new blocks. Might be worthwhile to meet to discuss the vision some more: I like some of the changes but a lot of it feels harder to use to me (possibly due to unfamiliarity or due to me not being the target user).
submitting a form, opening a dialog, canceling an action, or performing a delete | ||
operation. _Source: [W3C](https://www.w3.org/WAI/ARIA/apg/patterns/button/)_ | ||
|
||
<Canvas of={buttonStories.outlineButton} /> |
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 don't like that we're getting rid of the canvas. My primary workflow with storybook is putting the control in a certain state and looking at the rendered control or generated code. To be able to do that without crazy scrolling I need the canvas and controls to be near each other. Right now this randomly controls the Outline button canvas below which is too far away to be noticeable.
operation. _Source: [W3C](https://www.w3.org/WAI/ARIA/apg/patterns/button/)_ | ||
|
||
<Canvas of={buttonStories.outlineButton} /> | ||
<TableOfContents |
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 was a bit confused by the visual design of the ToC initially. Is it a breadcrumb with arrows instead of slashes between the items? Is it links with arrows next to them? What does the arrow signify again? (My first guess was external link but I guess not). Why isn't there an arrow next to the last link?

I sort of figured out answers to those by playing with it but I'm not sure I see how this is better than the ToC we had before.
/> | ||
|
||
## API | ||
## Implementation & API |
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.
Do you know the rationale behind adding the "Implementation" heading? I don't think we generally want to be adding implementation details in storybook.
- Primary Actions: Use buttons for primary actions that users need to take, such | ||
as submitting a form, saving changes, or proceeding to the next step. | ||
- Call to Action (CTA): Buttons are ideal for CTAs that encourage users to take | ||
a specific action, like βSign Up,β βBuy Now,β or β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.
This list seems incomplete. You use buttons for secondary and tertiary actions too (by my definition, "OK" is a primary action while "Cancel" would be a secondary action). And in toolbars and many other situations where there is no hierarchy/precedence.
I was also expecting the list to direct me to how to configure the button for each of these use cases but "CTA" isn't used elsewhere in the document and this use of "Primary" doesn't tie back to our appearance-variant='primary'
.
After reading a bit more would it make more sense for this to correspond to the numbered sub-headings under Appearance & Styling? Or just remove this list and let users discover the info below?
|
||
## Alternatives & Related | ||
|
||
### Alternative to Consider |
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 personally liked having the alternatives near the top because it quickly redirects users to the right component before they spend time reading the whole document.
Primary outline | ||
</NimbleButton> | ||
<NimbleButton appearance="block" appearanceVariant="primary"> | ||
Primary outline |
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.
@@ -1,45 +1,269 @@ | |||
import { Canvas, Meta, Controls, Title } from '@storybook/addon-docs/blocks'; |
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.
In my browser (macOS Firefox) the page load time seems not great.
Warm refresh of the production storybook is <1 second: https://nimble.ni.dev/storybook/?path=/docs/components-button--docs
But warm refresh of this page is 6-8 seconds: https://60e89457a987cf003efc0a5b-mgbjyxprll.chromatic.com/?path=/docs/components-button--docs
It's possible this is due to the chromatic URL / pre-deployment build but I'm worried the page might be heavier than before.
</div> | ||
</ExampleContainer> | ||
|
||
#### Unmistakably Clear |
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 think I need to hear the pitch for changing the tone to be more "marketing-y". "Unmistakably clear" / "The Stand Out" / "ideal when the Ghost button just isn't enough". I'm not getting a lot of value out of it.
|
||
<DocsGrid columns={2} minColumnWidth={300}> | ||
<GuidelinesDo title="Create a Clear Button Hierarchy" | ||
copy="Combine one or more Ghost button(s) with another single button of the available button styles (Outline, Block, Primary or Accent) to establish a clear hierarchy of actions, with the most critical action belonging to the non-ghost button of the set. "> |
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 guidance says to combine ghost with a single button of another style but the code combines it with multiple buttons of different styles.
href="?path=/docs/components-anchor-button--docs" | ||
title="Anchor Button" | ||
> | ||
Additional context or a short description can go 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.
We'll want to populate this placeholder content.
Pull Request
π€¨ Rationale
Initial implementation of an updated visual design for Storybook docs.
π©βπ» Implementation
Button
story as an exampleπ§ͺ Testing
β Checklist