The Wayback Machine - https://web.archive.org/web/20211117154324/https://github.com/storybookjs/storybook/issues/15632
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

Jump to bottom

addon-backgrounds: global context not initially updated with color #15632

Open
nickofthyme opened this issue Jul 20, 2021 · 4 comments
Open

addon-backgrounds: global context not initially updated with color #15632

nickofthyme opened this issue Jul 20, 2021 · 4 comments
Labels
addon: backgrounds bug good first issue help wanted P0 tiny
Milestone
6.4 improvements

Comments

Assignees
No one assigned
Labels
addon: backgrounds bug good first issue help wanted P0 tiny
Projects
None yet
Milestone
6.4 improvements
Linked pull requests

Successfully merging a pull request may close this issue.

Addon-backgrounds: Add inital color to globals
3 participants
@tmeasday @shilman @nickofthyme
@nickofthyme
Copy link

@nickofthyme nickofthyme commented Jul 20, 2021 •
edited by sync-by-unito bot
Loading

Describe the bug
When using @storybook/addons-backgrounds in a react project with a custom Decorator, the initial/default background value is not passed to the context.globals.

I have a project performing text contrast logic where I need the initial value to pass to javascript.

To Reproduce

  1. Pull down and run this repro here.
  2. Go to the primary button story.
  3. Notice the console.log of the context.globals shows and empty object.
  4. Click on a different background color, notice the context.globals now includes the selected value.

image

System

Environment Info:

  System:
    OS: macOS 11.4
    CPU: (16) x64 Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
  Binaries:
    Node: 14.17.3 - ~/.nvm/versions/node/v14.17.3/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 6.14.13 - ~/.nvm/versions/node/v14.17.3/bin/npm
  Browsers:
    Chrome: 91.0.4472.114
    Firefox: 90.0.1
    Safari: 14.1.1
  npmPackages:
    @storybook/addon-backgrounds: ^6.3.4 => 6.3.4
    @storybook/react: ^6.3.4 => 6.3.4

Additional context
Could be related to #14846
@nickofthyme nickofthyme mentioned this issue Jul 20, 2021
Open
@nickofthyme
Copy link
Author

@nickofthyme nickofthyme commented Jul 20, 2021 •
edited
Loading

After some digging, it looks like the so-called _initialGlobals are filtered by allowedGlobals which is determined from parameters.globals???? I don't see this documented anywhere in the docs, see code below.

storybook/lib/client-api/src/story_store.ts

Lines 216 to 228 in 209b6c4

finishConfiguring() {
this._configuring = false;
const { globals = {}, globalTypes = {} } = this._globalMetadata.parameters;
const allowedGlobals = new Set([...Object.keys(globals), ...Object.keys(globalTypes)]);
const defaultGlobals = Object.entries(
globalTypes as Record<string, { defaultValue: any }>
).reduce((acc, [arg, { defaultValue }]) => {
if (defaultValue) acc[arg] = defaultValue;
return acc;
}, {} as Args);
this._initialGlobals = { ...defaultGlobals, ...globals };

@ghengeveld could you shed some light on this per you changes in #15056?

Basically, adding globals.backgrounds like below enables the query param global value.

// preview.js
export const parameters = {
+  globals: {
+    backgrounds: {}
+  },
  backgrounds: {
// ...

Still doesn't solve the default case where no global param is present.

@ghengeveld seeing as you know the globals code fairly well, do you have any ideas why the globals from an addon would not be populated until the GLOBALS_UPDATED event is called?

Loading

@nickofthyme nickofthyme mentioned this issue Jul 20, 2021
Closed
@shilman
Copy link
Member

@shilman shilman commented Jul 21, 2021

this looks like a bug with addon-backgrounds or with core, depending on how you see it cc @yannbf @tmeasday @ghengeveld

also cc @stevensacks since you're using the same pattern in your i18n work

AFAICT what's going on here is that the globals code in core assumes that the globals are declared statically, either by the end user or by addons. what that would look like:

// .storybook/preview.js
export const globalTypes = { ... };
export const globals = { ... };

If you don't declare a globalType, it will infer the type from the data in globals. So the final results union these two.

addon-backgrounds uses a different pattern where the user declares global values as parameters, and then the addon calls updateGlobals on story render. I'm not a fan of this pattern, but it's what we have for now.

Given this pattern, we could fix the problem:

  1. The backgrounds-addon could declare the globalType that it's about to populate on its first render.
  2. The core code could be less strict about what it accepts.

I think the first fix is closer to the design intent of globals. WDYT?

Loading

@shilman shilman added this to the 6.4 improvements milestone Jul 21, 2021
@tmeasday
Copy link
Member

@tmeasday tmeasday commented Jul 21, 2021

I think I agree @shilman

Loading

@nickofthyme
Copy link
Author

@nickofthyme nickofthyme commented Jul 21, 2021

addon-backgrounds uses a different pattern where the user declares global values as parameters, and then the addon calls updateGlobals on story render. I'm not a fan of this pattern, but it's what we have for now.

@shilman based on the inability to statically define the initial background color are you okay with the changes in #15640?

Loading

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