Replace theme add-on with toolbar and global args #1219

tylersticka · 2021-05-14T21:45:04Z

Overview

After upgrading Storybook (#1214), our themes add-on became available in Docs view but was not functional. I attempted to use he add-on's withThemes decorator, but it does not appear to be compatible with inlined HTML stories. So this PR replaces it with a custom toolbar using the Toolbar add-on that ships with Storybook's essential add-ons.

This also helps us overcome an issue where we couldn't reliably configure a theme for a specific story. Now, we can specify a theme parameter corresponding to the desired theme class and the story will gain that theme. That means we can remove the fill-canvas template we were using as kind of a hack.

Screenshots

Testing

On the deploy preview…

Try the theme toolbar in a Canvas view
Try the theme toolbar in a Docs view

changeset-bot · 2021-05-14T21:45:07Z

⚠️ No Changeset found

Latest commit: 1e813ca

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Paul-Hebert · 2021-05-20T23:12:29Z

This is looking really good Tyler! I noticed a few oddities:

Broken Docs Pages

Some of our stories have broken docs pages with this change. For example, when I view one of our Prism docs pages I get a console error. Switching to the Canvas view shows that error in the canvas

Getting Stuck in a Theme

I'm not exactly sure why this happens but it seems like on the Input docs page specifically I can get stuck in a theme:

Bunch

For some reason the Bunch example disappears on the canvas in dark mode. Upping the z-index on .o-bunch fixes it. I'm not sure if this is a regression or an existing issue.

Table Theme Issues

This is unrelated to this PR, but the table has some issues in dark mode (low contrast caption color, incorrect striped color.)

What's the best place for me to report that? A new issue?

Other than that it's looking great! Let me know if you need any more info on any of this!

tylersticka · 2021-05-20T23:14:33Z

These are all great catches, @Paul-Hebert , thank you for testing so thoroughly!

src/components/button/button.stories.mdx

src/design/themes.stories.mdx

spaceninja · 2021-05-20T23:48:15Z

src/design/themes.stories.mdx

@@ -23,15 +13,17 @@ Themes may be applied to a containing element to change that content's appearanc
 The default theme, applied to the document `:root` by default.

 <Canvas>
-  <Story name="Light">{demoStory.bind({})}</Story>
+  <Story name="Light" args={{ theme: 'light' }}>


Should this be light or t-light?

In this case this is me telling the demo which theme is applied so it can modify its text and stuff accordingly, so even though it looks really close to the parameters it's not actually the same thing. I'm hoping this will only happen in the theme demos.

tylersticka · 2021-05-20T23:58:06Z

@Paul-Hebert I just pushed some updates that may address the issues you've observed...

Broken Docs Pages

I couldn't reproduce this error, but I added extra checks to make sure elements exist before using querySelector or classList in hopes that it will prevent this from occurring.

Getting Stuck in a Theme

I also couldn't reproduce this error, but I noticed a different console error relating to my use of value: undefined which could, theoretically, get the toolbar stuck. I changed to null, which seemed to eliminate the warning... but it may have nothing to do with what you were seeing.

Bunch

I could reproduce this one! I think I fixed it by applying the theme class to document.body instead of document.documentElement.

Table Theme Issues

I've created an issue to resolve this: #1229

spaceninja · 2021-05-21T00:00:16Z

I'm seeing the same problem @Paul-Hebert saw on the Input page — changing the theme to dark on the Input docs page doesn't change anything.

I also still can't see the bunch icons in either theme on the Bunch docs pages.

I'm testing in Edge, BTW

tylersticka · 2021-05-21T00:03:33Z

I'm seeing the same problem @Paul-Hebert saw on the Input page — changing the theme to dark on the Input docs page doesn't change anything.

Okay, now I can reproduce. Not sure what I'm doing differently now. I think this may have something to do with inline versus iframed stories.

Thanks, @spaceninja! Sorry for the runaround.

spaceninja · 2021-05-21T00:05:59Z

I'm not sure if this is a bug, because I'm not sure it's a real-world scenario, but if I apply the global dark theme on the Themes docs page, the nested dark card inside the light theme (that is now dark theme) remains dark. I would expect that if one changes, the other does also, but feel free to disregard if this isn't a situation a real user could end up in.

tylersticka · 2021-05-21T00:10:32Z

I'm not sure if this is a bug, because I'm not sure it's a real-world scenario, but if I apply the global dark theme on the Themes docs page, the nested dark card inside the light theme (that is now dark theme) remains dark. I would expect that if one changes, the other does also, but feel free to disregard if this isn't a situation a real user could end up in.

Good catch. I think at the very least we could force them to stay the same theme regardless, since this is kind of a special case and someone could (as you suggest) swap themes and then head over here.

spaceninja · 2021-05-21T00:28:52Z

I think at the very least we could force them to stay the same theme regardless

Could we disable the theme control on that page?

tylersticka · 2021-05-21T00:32:20Z

Gonna have to pick this up tomorrow, but there may be a major blocker to getting this working with iframed docs: storybookjs/storybook#13444

It doesn't even seem like the iframed docs are responding to a change in that toolbar, but I'd like to take a new look with fresh eyes tomorrow.

tylersticka · 2021-05-21T00:32:33Z

Could we disable the theme control on that page?

Probably!

tylersticka · 2021-05-21T16:11:38Z

I think this bug may be more closely related to the issue we're seeing on Input: storybookjs/storybook#14477

gerardo-rodriguez · 2021-05-21T16:19:44Z

@tylersticka I can't tell the state of this PR at a glance. Are you in the process of addressing feedback or is the PR in a ready state for another review. Let me know how I can help! 🙂

tylersticka · 2021-05-21T16:22:30Z

I can't tell the state of this PR at a glance. Are you in the process of addressing feedback or is the PR in a ready state for another review. Let me know how I can help! 🙂

@gerardo-rodriguez I'm in the process of addressing feedback at the moment! I just removed the open request for Dev review, sorry about that!

tylersticka · 2021-05-21T17:00:12Z

Okay, I think this is ready for re-review, with a few caveats:

I can't easily disable the toolbar for specific story files. I've overtly set the theme on both theme stories to address this comment.
I couldn't find a solution so that themes would work for non-inlined stories in Docs mode. I've updated the custom decorator comments to include more information about why this is a struggle.

Both issues are solvable if we wanted to expand this from a custom toolbar and decorator to a full add-on, which would give us the ability to assign more complicated actions to the toolbar items and would allow us to force-refresh stories in contexts like the docs mode. I haven't done this because...

The remaining issues are known and documented in Storybook with some dev effort already allocated to them. It's likely that these issues will be trivial to address in a future Storybook update.
Every single existing add-on for this that's compatible with Storybook 6.x has more bugs than this implementation (including what we were using before, which didn't work in Docs mode at all for HTML or Web Component "frameworks"), which suggests to me that this is a non-trivial problem to fully solve.
The majority of our pattern stories are inlined, as that is the default. Of the seven patterns that are not inlined, three are layout objects, two are theme containers themselves (Sky Nav and Cloud Cover), one is only optimized for a light theme (Ground Nav), and the last pattern (Input) can be safely inlined once we have more form layouts to better visualize the alignment of inputs, labels and buttons (see Email signup form pattern(s) #1088 and Comment form pattern(s) - Development #1216).

TL;DR: This isn't perfect, but it's a lot better than our current solution, and is likely to improve more as our dependencies update and our project evolves. But let me know if you disagree!

gerardo-rodriguez · 2021-05-21T18:04:29Z

.storybook/theme-decorator.js

+        const previewElement = storyElement.closest('.docs-story');
+        updateTheme(previewElement, theme);


I don't yet have an understanding of story elements and their contents. Is it a guaranty that there will always exist a .docs-story or do we need to check that previewElement is not null before attempting to update the theme? 🤔 ¯\_(ツ)_/¯

This seems like a good safety check given that the underlying Storybook markup is outside our control

The updateTheme function does check that the element is truthy and has a classList property, so I don't think another check before calling it is necessary.

Paul-Hebert

TL;DR: This isn't perfect, but it's a lot better than our current solution, and is likely to improve more as our dependencies update and our project evolves. But let me know if you disagree!

That makes sense to me 👍 I'm cool with merging this in, though agree with Gerardo that it would be nice to add a safety check to confirm the .story-docs element exists before interacting with it.

tylersticka marked this pull request as ready for review May 14, 2021 22:00

tylersticka requested a review from a team May 14, 2021 22:00

tylersticka added 5 commits May 20, 2021 16:28

Uninstall storybook-addon-themes

a3ea4aa

Add custom theme toolbar

4d54454

Account for changing tabs with theme selected

d401906

Remove old fill-canvas theme preview technique

776b981

Possibly fix theme decorator issues

52bf4ff

tylersticka force-pushed the chore/update-storybook-theme-addon branch from 762b545 to 52bf4ff Compare May 20, 2021 23:45

Theme body instead of rootElement

2350e69

tylersticka mentioned this pull request May 20, 2021

Optimize Table component for dark theme #1229

Closed

spaceninja reviewed May 20, 2021

View reviewed changes

tylersticka requested a review from a team May 20, 2021 23:58

Overtly specify light theme

211c439

tylersticka removed the request for review from a team May 21, 2021 16:22

Note unresolved issue in code comments

1e813ca

tylersticka requested a review from a team May 21, 2021 17:00

tylersticka changed the title ~~Revise theme add-on implementation~~ Replace theme add-on with toolbar and global args May 21, 2021

gerardo-rodriguez reviewed May 21, 2021

View reviewed changes

Paul-Hebert self-requested a review May 21, 2021 19:49

Paul-Hebert approved these changes May 21, 2021

View reviewed changes

tylersticka merged commit 5c53e26 into v-next May 21, 2021

tylersticka deleted the chore/update-storybook-theme-addon branch May 21, 2021 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace theme add-on with toolbar and global args #1219

Replace theme add-on with toolbar and global args #1219

tylersticka commented May 14, 2021

changeset-bot bot commented May 14, 2021 •

edited

Loading

Paul-Hebert commented May 20, 2021

tylersticka commented May 20, 2021

spaceninja May 20, 2021

tylersticka May 20, 2021

tylersticka commented May 20, 2021

spaceninja commented May 21, 2021 •

edited

Loading

tylersticka commented May 21, 2021

spaceninja commented May 21, 2021

tylersticka commented May 21, 2021

spaceninja commented May 21, 2021

tylersticka commented May 21, 2021

tylersticka commented May 21, 2021

tylersticka commented May 21, 2021

gerardo-rodriguez commented May 21, 2021

tylersticka commented May 21, 2021

tylersticka commented May 21, 2021

gerardo-rodriguez May 21, 2021

Paul-Hebert May 21, 2021

tylersticka May 21, 2021

Paul-Hebert left a comment

		const previewElement = storyElement.closest('.docs-story');
		updateTheme(previewElement, theme);

Replace theme add-on with toolbar and global args #1219

Replace theme add-on with toolbar and global args #1219

Conversation

tylersticka commented May 14, 2021

Overview

Screenshots

Testing

changeset-bot bot commented May 14, 2021 • edited Loading

⚠️ No Changeset found

Paul-Hebert commented May 20, 2021

Broken Docs Pages

Getting Stuck in a Theme

Bunch

Table Theme Issues

tylersticka commented May 20, 2021

spaceninja May 20, 2021

Choose a reason for hiding this comment

tylersticka May 20, 2021

Choose a reason for hiding this comment

tylersticka commented May 20, 2021

spaceninja commented May 21, 2021 • edited Loading

tylersticka commented May 21, 2021

spaceninja commented May 21, 2021

tylersticka commented May 21, 2021

spaceninja commented May 21, 2021

tylersticka commented May 21, 2021

tylersticka commented May 21, 2021

tylersticka commented May 21, 2021

gerardo-rodriguez commented May 21, 2021

tylersticka commented May 21, 2021

tylersticka commented May 21, 2021

gerardo-rodriguez May 21, 2021

Choose a reason for hiding this comment

Paul-Hebert May 21, 2021

Choose a reason for hiding this comment

tylersticka May 21, 2021

Choose a reason for hiding this comment

Paul-Hebert left a comment

Choose a reason for hiding this comment

changeset-bot bot commented May 14, 2021 •

edited

Loading

spaceninja commented May 21, 2021 •

edited

Loading