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-backgrounds: global context not initially updated with color #15632

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

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

nickofthyme opened this issue Jul 20, 2021 · 9 comments

Comments

@nickofthyme
Copy link

nickofthyme commented Jul 20, 2021

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
Copy link
Author

nickofthyme commented Jul 20, 2021

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.

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?

@shilman
Copy link
Member

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?

@tmeasday
Copy link
Member

I think I agree @shilman

@nickofthyme
Copy link
Author

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?

@shilman shilman removed this from the 6.4 improvements milestone May 20, 2022
@bytrangle
Copy link
Contributor

@nickofthyme Maybe I don't understand the issue very well, but in your demo, you tried to get the value of context.globals in a decorator function. But you didn't declare a globalTypes in .storybook/preview.js. So context.globals is empty on initial rendering. Isn't this expected?

@nickofthyme
Copy link
Author

nickofthyme commented Jun 10, 2022

@bytrangle the issue is that it does not capture the initial default value in preview.js here, and pass it to the decorators.

The whole globalTypes vs globals is really confusing when to use one or the other or both. As @shilman said

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

Is this not true? If it is I'd think the demo would work as is.


All in all how do you make the demo work so that it passes the initial default value to the decorators? In my custom background addon I have to call setGlobals for it to capture the initial value.

@shilman shilman removed the P0 label Oct 18, 2022
@ndelangen
Copy link
Member

Is this still an issue with 7.0 beta?

@lmcgartland
Copy link

lmcgartland commented Mar 12, 2023

+1 I'm still seeing this issue with Svelte stories

@elussich-globant
Copy link

+1 Still experiencing this issue. Would love to get defaults working on a per Story basis, this would save our dev team lot of boilerplate for setting up background color styles for every Story.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants