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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Theming: Add theme variable to set the preview background color #24575

Merged
merged 3 commits into from
Oct 31, 2023

Conversation

JReinhold
Copy link
Contributor

@JReinhold JReinhold commented Oct 25, 2023

Closes #22029

What I did

This PR introduces a new theming variable, appPreviewBg to explicitly set the preview background color. This was previously possible with appContentBg in 6.5, but that was problematic as documented in #21080.

We generally recommend setting explicit backgrounds in the rendered stories directly (eg. with a decorator in preview.ts), but doing this directly in the manager is now also possible with appPreviewBg.

an open question is whether or not the "loading preview" element should have the same preview background or maintain the current app background. in dark mode, the current app background is dark so the loading screen will be dark for a short period of time before going white. if we changed it to match, it would be white all the time.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

See this demonstrated in different scenarios in this sandbox: https://stackblitz.com/edit/github-u7boko?file=.storybook%2Fpreview.ts

  1. @storybook/addon-themes has been added to demonstrate that any background set using that addon will still override whatever background set in the manager with the theme variable. Story/preview level styles should always override manager styles. You need to disable this addon to experience the next two scenarios.
  2. storybook-dark-mode has been added to demonstrate that it still works to set manager backgrounds from addons, if the user specifies the new theme variable in the parameters (as documented by the addon). You need to disable this addon to experience the next scenario
  3. Users can now set the preview background directly with the usual addons.setConfig API. (Changing this requires restarting the SB server)
  4. Finally disable all of the above to see that previews still default to a white background, both when Storybook is in dark and light mode - use the browser DevTools to toggle between the two and reload the site).

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for 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:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

馃 Canary release

This pull request has been released as version 0.0.0-pr-24575-sha-94eee1ff. Install it by pinning all your Storybook dependencies to that version.

More information
Published version 0.0.0-pr-24575-sha-94eee1ff
Triggered by @JReinhold
Repository storybookjs/storybook
Branch 22029-bug-preview-always-has-a-white-background
Commit 94eee1ff
Datetime Wed Oct 25 21:46:11 UTC 2023 (1698270371)
Workflow run 6646514771

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=24575

@JReinhold JReinhold linked an issue Oct 25, 2023 that may be closed by this pull request
@JReinhold JReinhold changed the title Theming: Enabling setting the preview background color with new theme variable Theming: Enable setting the preview background color with new theme variable Oct 25, 2023
@JReinhold JReinhold self-assigned this Oct 25, 2023
@JReinhold JReinhold marked this pull request as ready for review October 25, 2023 11:27
@shilman shilman changed the title Theming: Enable setting the preview background color with new theme variable Theming: Add theme variable to set the preview background color Oct 25, 2023
@JReinhold JReinhold mentioned this pull request Oct 25, 2023
8 tasks
@JReinhold
Copy link
Contributor Author

@yannbf I've changed the loader to now also have the preview background. This doesn't eliminate the flash of white entirely unfortunately. The flow is this:

  1. show preview loader, with the theme background
  2. the preview is loaded, now we load the actual story. this is with a white background
  3. the story is loaded and in most cases has a transparent background, thus getting the theme background again because it is set on the iframe.

So if you set appPreviewBg: 'red' you get red馃憠white馃憠red because of step 2, which is because of this line that I don't really want to touch right now. basically it's a way to cover up the story while it is still loading:

In this video I'm demonstrating loading a story a few times, then loading docs, and finally I "slow it down" by enabling the debugger to pause at each step outlined above.

Export-1698270790261.mp4

I think this is the best we can do right now, and it's an improvement over what we have.

@yannbf
Copy link
Member

yannbf commented Oct 26, 2023

I think this needs some eyes from @MichaelArestad and @cdedreuille

@cdedreuille
Copy link
Contributor

The solution seems good to me as a quick fix for now but this highlight how crucial it it for us to simplify how to manage theming. If we move away from using JS to set colors, then you would never have any flashing problems. I really hope we can bring the Theming 2.0 project to Storybook 8.0 #24344

Regarding naming, I'm not 100% convinced that people will understand the difference between appContentBg and appPreviewBg. We should find a section in the docs to explain the difference.

@JReinhold
Copy link
Contributor Author

Regarding naming, I'm not 100% convinced that people will understand the difference between appContentBg and appPreviewBg. We should find a section in the docs to explain the difference.

I agree the naming is not perfect, because now appContentBg only affects the toolbar and the panel and a few other places. I don't think we can change that now though, unfortunately.

@JReinhold JReinhold merged commit 8fdd30a into next Oct 31, 2023
55 checks passed
@JReinhold JReinhold deleted the 22029-bug-preview-always-has-a-white-background branch October 31, 2023 12:01
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.

[Bug]: Preview always has a white background
3 participants