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

[Storybook] General consistency pass #7245

Merged
merged 12 commits into from
Oct 3, 2023
Merged

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Oct 2, 2023

Summary

This PR is a general cleanup/consistency pass on our existing stories. The way we've written stories has evolved as we've learned more tips and tricks of Storybook's API, and this attempts to elevate all existing stories to the same level.

  1. Adds new hideStorybookControls() and disableStorybookControls() utils so we're not copying the same { table: { disable: true } } or { control: false } over and over again

    • This has the main DRY benefit of future-proofing, if someday Storybook changes the API they use to render the controls table, we can quickly update it in one place instead of find+replacing hundreds of lines
  2. Standardizes how and where we list component defaults - this should now always be in the initial component meta.args as opposed to defining them as separate consts and spreading them repeatedly.

    • This has the added benefit of receiving automatic prop name/type checking as opposed to manual.
  3. More opinionated cleanups of a handful of spicier components

As always, I recommend following along with changes by commit, as the final diff touches a lot of files and I'm doing a few different separate clean up things here.

QA

General checklist

N/A, dev only

- to try match how Storybook will yell if an argTypes is passed with invalid props
- just in case, + helpful for developer documentation
- these get spread to all stories in the file, and also get correctly typed automatically - no need to declare / spread an external variable

- I totally forgot this was a thing we could do and would like to adopt the pattern going forward
- these changes are a bit more complex than the previous copy/paste commit

- e.g. there's some args mixed in that are not defaults
- or, e.g. all args are not defaults, but are repeated between stories for testing
- split component into separate stories for single vs. multi - consumers won't (shouldn't) be swapping types dynamically in any case, and it really angers typescript

+ remove `data-test-subj` param/control, we already handle this across all stories
- it was the first and one and therefore the worst one, and we are much better at writing stories now
@cee-chen cee-chen added testing Issues or PRs that only affect tests - will not need changelog entries skip-changelog tech debt labels Oct 2, 2023
@cee-chen cee-chen requested a review from breehall October 2, 2023 21:10
@cee-chen cee-chen marked this pull request as ready for review October 2, 2023 21:10
@cee-chen cee-chen requested a review from a team as a code owner October 2, 2023 21:10
@cee-chen
Copy link
Member Author

cee-chen commented Oct 2, 2023

@breehall Assigning this your way as you've been in the Storybook space these past few weeks. No rush at all on this (and of course lmk if you have any feedback/preferences on code style!)

- remove unnecessary "multiple fixed headers" story - that logic now lives in EuiHeader and already has its own story

- move flyout story to the end, since it doesn't have anything to control and it's a visual demo

- move localstorage example up, since Kibana is actually using it, + trim controls to only relevant ones

- remove unused controls in global CSS variable story, and fix bottom bar to respect left/right control
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

Copy link
Contributor

@breehall breehall left a comment

Choose a reason for hiding this comment

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

[Adding my comments in a single thread because I'm bad at going back and forth between pages]

  1. The new DRY helpers are very handy and significantly cut down the bloat and repetitiveness of hiding controls.
  2. I see that we have a TO DO task to figure out how to remove the "set up" text when we disable controls. That will be handy because that link just takes us to Storybook documentation right now.
  3. I like that the component defaults were added to the Meta object. It's one less thing to maintain.
  4. Good call on splitting up the EuiButtonGroup stories into single select & multi select
  5. Now that we've standardized certain patterns, we'll probably want to start a wiki and maybe even create a script to generate the story structure for us. It would probably be helpful. I can throw together some items in a draft wiki

@cee-chen
Copy link
Member Author

cee-chen commented Oct 3, 2023

I see that we have a TO DO task to figure out how to remove the "set up" text when we disable controls. That will be handy because that link just takes us to Storybook documentation right now.

Just to add a note, I don't think Storybook currently supports that kind of flexibility without it being a massive pain (literally having to go into either vanilla JS to manipulate the DOM or some other hacky workaround). There's a fair amount of open issues in the Storybook repo around hiding controls (if we really wanted to, contributing back to their repo might actually be the cooler option).

I can throw together some items in a draft wiki

That would be amazing!! thanks ❤️

@cee-chen cee-chen merged commit ce51332 into elastic:main Oct 3, 2023
7 checks passed
@cee-chen cee-chen deleted the storybook-utils branch October 3, 2023 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog tech debt testing Issues or PRs that only affect tests - will not need changelog entries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants