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

Addon: Create @storybook/addon-themes #23524

Merged
merged 22 commits into from
Jul 25, 2023
Merged

Addon: Create @storybook/addon-themes #23524

merged 22 commits into from
Jul 25, 2023

Conversation

ShaunEvening
Copy link
Contributor

@ShaunEvening ShaunEvening commented Jul 19, 2023

What changed

  • Port theme switcher from styling addon into mono repo
  • Write docs for addon in the essentials addon docs
  • Match setup to other core addons
  • Add theme addon to essentials package
  • ???

How to test

  • ???

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "build", "documentation", "maintenance", "dependencies", "other"]

@ShaunEvening ShaunEvening self-assigned this Jul 19, 2023
@ShaunEvening ShaunEvening changed the title Create @storybook/addon-themes Addon: Create @storybook/addon-themes Jul 19, 2023
@cdedreuille
Copy link
Contributor

Fantastic job @integrayshaun. Something that would be great is to improve the gif for 7.0 as it still has the old tabs I believe. But not a blocker at all I believe. We can update that later.

Copy link
Contributor

@jonniebigodes jonniebigodes left a comment

Choose a reason for hiding this comment

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

@integrayshaun looking great. Left one starter item for you to look at for now. One thing, though, when you address my comment and before pushing the commit, can you run yarn pretty-docs so that we don't get any errors with the snippets?

Also, a couple of things:

  • Don't forget to update the toc file to get it referenced in the documentation
  • As this will be a part of essentials, the introduction page should also be updated to factor in this one.

Let me know once you've addressed the feedback, and we'll take it from there.
@cdedreuille good call on that, but don't worry, I can take care of it and align it with the rest of the videos/gifs we currently have. I'll follow up @integrayshaun on this.

docs/essentials/themes.md Outdated Show resolved Hide resolved
@ShaunEvening ShaunEvening marked this pull request as ready for review July 20, 2023 19:51

Storybook Addon Themes can be used which between multiple themes for components inside the preview in [Storybook](https://storybook.js.org).

![React Storybook Screenshot](https://user-images.githubusercontent.com/42671/98158421-dada2300-1ea8-11eb-8619-af1e7018e1ec.png)
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is not the screenshot you want to use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, Joao is going to help me out with an optimized video like we have for the other addons

code/addons/themes/docs/getting-started/bootstrap.md Outdated Show resolved Hide resolved
code/addons/themes/docs/getting-started/emotion.md Outdated Show resolved Hide resolved
code/addons/themes/docs/getting-started/tailwind.md Outdated Show resolved Hide resolved
code/addons/themes/docs/getting-started/tailwind.md Outdated Show resolved Hide resolved
code/addons/themes/package.json Outdated Show resolved Hide resolved
code/addons/themes/package.json Show resolved Hide resolved
docs/toc.js Show resolved Hide resolved
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Minor nits. Otherwise looking pretty good!

@valentinpalkovic
Copy link
Contributor

image
The addon does not appear in the bar panel, when not explicitly mentioned in the addons section of main.ts. Shouldn't it be part of @storybook/addon-essentials?

@ShaunEvening
Copy link
Contributor Author

@valentinpalkovic

Docs don't turn dark in dark mode
This is due to the specificity of the markdown css and the fact that peoples theme styles aren't likely to have rules for the css escape-hatch classes.

The toolbar item doesn't always appear
The toolbar item hides itself unless there are two or more themes registered with the decorator

@valentinpalkovic
Copy link
Contributor

valentinpalkovic commented Jul 24, 2023

@integrayshaun there are two themes registered. As I said, it does not appear in the panel, if the addon is not explicitly mentioned in the addons array in main.ts. Shouldn't it also work with mentioning only @storybook/addon-essentials in the addons array?

@valentinpalkovic
Copy link
Contributor

valentinpalkovic commented Jul 24, 2023

The docs limitation should be mentioned somewhere in the docs, if not already done.

@ShaunEvening
Copy link
Contributor Author

@valentinpalkovic Weird! It definitely should show even in essentials 🤔 I'll try to test after my planning meeting here.

I'll add the docs limitation into the docs as well :) Thank you for testing 🙏

@valentinpalkovic
Copy link
Contributor

valentinpalkovic commented Jul 24, 2023

You’re welcome :) tested it with next.js in combination with material ui theming

docs/essentials/themes.md Outdated Show resolved Hide resolved
Copy link
Contributor

@kylegach kylegach left a comment

Choose a reason for hiding this comment

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

Docs look good!

ndelangen and others added 4 commits July 25, 2023 12:56
Co-authored-by: Valentin Palkovic <valentin@chromatic.com>
Co-authored-by: Kyle Gach <kyle.gach@gmail.com>
Copy link
Contributor

@valentinpalkovic valentinpalkovic left a comment

Choose a reason for hiding this comment

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

LGTM!

code/addons/themes/README.md Outdated Show resolved Hide resolved
@ShaunEvening ShaunEvening merged commit aa09ec1 into next Jul 25, 2023
@ShaunEvening ShaunEvening deleted the addon-themes branch July 25, 2023 16:56
@ShaunEvening ShaunEvening removed the patch:yes Bugfix & documentation PR that need to be picked to main branch label Jul 25, 2023
@github-actions github-actions bot mentioned this pull request Jul 26, 2023
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants