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.

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

chore(demo): refactor storybook #1265

Merged
merged 80 commits into from
Jan 7, 2022
Merged

chore(demo): refactor storybook #1265

merged 80 commits into from
Jan 7, 2022

Conversation

jzempel
Copy link
Member

@jzempel jzempel commented Dec 28, 2021

Description

This PR represents a pretty massive overhaul of the Storybook component demos. You might find that reading through the new demo documentation is the best way to get acquainted with the refactored code.

Detail

Highlights include:

  • a substantial uptick in Story controls with categorized differentiation between main element props, subcomponents, and "Story" controls
  • moved stories to package /demo folders to better align with other Garden repos
  • standardized MDX-to-TSX Story configuration
  • added Figma designs (all currently available) to story tabs
  • removal of outmoded documentation (the website is Garden's documentation source of truth)

(@smritimadan this recent staging deployment can be used, as needed, for ongoing API doc recovery efforts.)

Checklist

  • 👌 design updates are Garden Designer approved (add the
    designer as a reviewer)
  • 🌐 demo is up-to-date (yarn start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 🤘 renders as expected with Bedrock CSS (?bedrock)
  • ♿ analyzed via axe and evaluated using VoiceOver
  • 💂‍♂️ includes new unit tests
  • 📝 tested in Chrome, Firefox, Safari, Edge, and IE11

@jzempel
Copy link
Member Author

jzempel commented Jan 4, 2022

It looks like a few stories didn't make it out the same

The stories are definitely not the same, nor intended to be. The updates fall in line with the new demo.md documentation, pulling out styled-components that leave false impressions of default component renderings, initially rendering with prop defaults (while providing 100% control), and ensuring that the website will be the true source of documented component usage truth.

Copy link
Contributor

@hzhu hzhu left a comment

Choose a reason for hiding this comment

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

This is a huge improvement. I love the new structure! Thanks @jzempel!

@exelarios
Copy link
Contributor

exelarios commented Jan 5, 2022

I noticed with Typography/paragraph it displays MD tags, not entirely sure where that's coming from?

image

@jzempel
Copy link
Member Author

jzempel commented Jan 5, 2022

I noticed with Typography/paragraph it displays MD tags, not entirely sure where that's coming from?

Similar to the website this is indicating that MD typography is meant to be used with medium sized Paragraph and so on. From the perspective of Garden design, there is no other valid combination (although, technically, a large-spaced Paragraph could use SM typography).

@mtomcal
Copy link

mtomcal commented Jan 6, 2022

There seems to be styling divergence between old Grid and new Grid docs:

Old
Screen Shot 2022-01-06 at 2 24 54 PM
New
Screen Shot 2022-01-06 at 2 24 44 PM

Was this intended?

@jzempel
Copy link
Member Author

jzempel commented Jan 6, 2022

Was this intended?

@mtomcal yes, this Grid demo is rendering as intended. Demos should render the default state initially and then offer controls (in this case, debug) to change, well, basically just about everything.

See previous #1265 (comment) re: similar observation.

@mtomcal
Copy link

mtomcal commented Jan 6, 2022

Was this intended?

@mtomcal yes, this Grid demo is rendering as intended. Demos should render the default state initially and then offer controls (in this case, debug) to change, well, basically just about everything.

See previous #1265 (comment) re: similar observation.

When I see this, I ask myself, "what am I looking at?". I am not sure when I see this. Any chance this could be documented in the story itself about the presence of the "debug" toggle?

@jzempel
Copy link
Member Author

jzempel commented Jan 6, 2022

Any chance this could be documented in the story itself about the presence of the "debug" toggle?

My preference is to keep the component demos essentially doc-free. Given that the Grid demo in particular has caused repeated concern, how would you feel about making an exception for the "default state" rule and allowing debug=true in this one, isolated case?

@mtomcal
Copy link

mtomcal commented Jan 6, 2022

It might be easiest to show the "Docs" tab first then the "Canvas" tab which documents the "debug" toggle front and center. This gives me context and information about this more abstract demo.

Screen Shot 2022-01-06 at 3 11 08 PM

Another storybook from the Azure team shows the "Docs" first then the "Preview" (similar to "Canvas") tab.
https://azure.github.io/communication-ui-library/?path=/docs/ui-components-gridlayout--grid-layout

This might also help document the case mentioned here as well: #1265 (comment)

@mtomcal
Copy link

mtomcal commented Jan 6, 2022

Aside from documenting how to use the demo, this is an amazing update and epic reorganization of the stories. Im looking forward to using this for Splitter component. 👏

@jzempel
Copy link
Member Author

jzempel commented Jan 6, 2022

It might be easiest to show the "Docs" tab first

Garden determined early on that the Canvas was most valuable to the greater audience (design + dev). I'm not keen to reverse that decision here.

@mtomcal
Copy link

mtomcal commented Jan 7, 2022

@jzempel What would you suggest on messaging "how to use this abstract demo" to the developer?

@jzempel
Copy link
Member Author

jzempel commented Jan 7, 2022

What would you suggest on messaging...

@mtomcal sorry, I'm not tracking with you. We're not super interested in documenting "how to use this" as it is primarily a tool for Garden visual testing and the how-to boils down to familiarity with Storybook 6.x. The Garden team is expected to be familiar with Storybook 6.x.

@mtomcal
Copy link

mtomcal commented Jan 7, 2022

@jzempel Let's take this offline then and follow up there. Thanks!

Copy link
Contributor

@Francois-Esquire Francois-Esquire left a comment

Choose a reason for hiding this comment

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

LGTM 🙌 thanks for this huge effort @jzempel

hint = 'Hint',
hasMessage = true,
message = 'Message',
validation,
Copy link
Contributor

Choose a reason for hiding this comment

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

I couldn't tell where this was defined in the story. Can you point me to where it can be found?

Copy link
Member Author

Choose a reason for hiding this comment

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

In dropdowns (and fields) validation is a concept that spans both the input and associated Message component. In the case you referenced, it applies styling to both the Combobox and associated Field > Message component. The same control can be seen with dropdowns Autocomplete, Multiselect, Select and similar "Forms" components.

We don't have solid design evidence that validation should apply (as it currently does) disparate from input & message. But the current implementation allows for cases we don't know about.

Copy link
Member Author

Choose a reason for hiding this comment

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

p.s. The MDX story does not (should not) need to expose props as args which are inherent in the component identified in <Meta>. Which, in this case, is Combobox and includes validation, focusInset, isBare, isCompact, etc.

// Reset Downshift refs on every render
itemIndexRef.current = 0;
nextItemsHashRef.current = {};
previousIndexRef.current = undefined;
itemSearchRegistry.current = [];

const popperPlacement = isRtl(props)
const popperPlacement = themeContext.rtl
Copy link
Contributor

@Francois-Esquire Francois-Esquire Jan 7, 2022

Choose a reason for hiding this comment

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

Do you remember if there are any more instances of isRtl we should modernize or did we get them all? We could track the rest for a future date if there are any left.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we got em' all 😅. My "element" component typedef work will tell us for certain. And we'll follow-on deprecate isRtl for removal in v9.

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

Successfully merging this pull request may close these issues.

7 participants