Skip to content

Conversation

fredvisser
Copy link
Contributor

Pull Request

🀨 Rationale

Initial implementation of an updated visual design for Storybook docs.

πŸ‘©β€πŸ’» Implementation

  • Created a handful of React layout components:
    • Leveraged new Figma component designs and copilot to largely autogenerate components
    • For the TOC component – based the implementation on the OOTB TOC component, but updated to match the new design
  • Updated the Button story as an example

πŸ§ͺ Testing

  • Manual review of the Button story

βœ… Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

Copy link
Contributor

@jattasNI jattasNI left a 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} />
Copy link
Contributor

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
Copy link
Contributor

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?

image

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
Copy link
Contributor

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.

Comment on lines +45 to +48
- 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.”
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what's going on with the styling here but I don't think black text on a dark grey background is correct.

image

@@ -1,45 +1,269 @@
import { Canvas, Meta, Controls, Title } from '@storybook/addon-docs/blocks';
Copy link
Contributor

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
Copy link
Contributor

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. ">
Copy link
Contributor

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.
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants