Storybook: Theme variables(brandTitle, brandUrl, brandImage) not working

[lonyele]

Describe the bug
Even if I set brandTitle, brandUrl, brandImage on theme object, It doesn't work at all(5.1.0-alpha.27)

To Reproduce
Set any combinations of brandTitle, brandUrl, brandImage of defined, undefined, null on theme object.

Expected behavior
SidebarHeading should be rendered according to the inputs from options

System:

• OS: Windows10, Ubuntu 18.04
• Browser: chrome, firefox
• Framework: react
• Version: 5.1.0-alpha.27

[KaboomFox]

It also doesn't work in 5.0.6

[lonyele]

@KaboomFox Just FYI, reason for not working is different for version 5.0.6 and this 5.1.0-alpha.27.

Released version 5.0.6 had a bug and PR that fixes merged to 5.1.0.-alpha.7 which is not released yet.

Problem with this version(5.1.0-alpha.27 actually from 5.1.0-alpha.10) is that It is not working at all.

[KaboomFox]

👍

[shilman]

@lonyele So if I cherry-pick https://github.com/storybooks/storybook/pull/6120 into master do you think it will fix 5.0.x?

[lonyele]

@shilman Yes, If you cherry-pick into master then It will fix for 5.0.x (solve #5866 #6476 )

However, If you release 5.1.x then the bug will arise with the exact same problem I stated here.

[shilman]

Thanks for the clarification @lonyele !

I'll release the patch and follow-up to make sure that the new bug gets fixed in 5.1.x

감사합니다 🙏

[lonyele]

@shilman Whaaaaaat 안녕하세요~ Recently, I've started trying to contribute to open source as a mean of learning. Let's see how it goes

[shilman]

@lonyele 화이팅! ㅋㅋㅋㅋ

[hipstersmoothie]

I just cleared my localstorage and this is what the brand section looks like

[shilman]

@hipstersmoothie I'm releasing a new version now with the fix from #6120. Can you try again in a few mins?

[hipstersmoothie]

Yeah sure. I'm stepping through the code and it seems like my theme is represented in two different places in the state.

you can see ui.theme has the brandImage set properly and on the root (theme) does not

[hipstersmoothie]

Also I'm currently on the latest storybook alpha

[hipstersmoothie]

seems to be something to do with checkDeprecatedThemeOptions(options)

[hipstersmoothie]

this line always returns true https://github.com/storybooks/storybook/blob/f07b2b7574611388ef45a4cb8a89389b434e257d/lib/api/src/modules/layout.ts#L119

which overrides all of these options on this line

https://github.com/storybooks/storybook/blob/f07b2b7574611388ef45a4cb8a89389b434e257d/lib/api/src/modules/layout.ts#L275

[hipstersmoothie]

Also from my reading of the code it seems like the keys of deprecatedThemeOptions are the deprecations and not the values. I think the appropriate code is

const checkDeprecatedThemeOptions = (options: Options) => {
  // Check the deprecated options exist
  if (Object.keys(deprecatedThemeOptions).find(v => !!options[v])) {
    return applyDeprecatedThemeOptions(options);
  }
  return {};
};

[hipstersmoothie]

applyDeprecatedThemeOptions would also need to be changed because it will override the configured theme.

const applyDeprecatedThemeOptions = deprecate(
  ({ name, url }: Options, userOptions: any): PartialThemeVars => ({
    brandTitle: userOptions.brandTitle || name,
    brandUrl: userOptions.brandUrl || url,
    brandImage: userOptions.brandImage || null,
  }),
  deprecationMessage(deprecatedThemeOptions)
);

[hipstersmoothie]

@shilman PR opened https://github.com/storybooks/storybook/pull/6543

[hipstersmoothie]

My PR seems to fix the brandTitle and brandUrl but the image is still not there

[hipstersmoothie]

It is this line

https://github.com/storybooks/storybook/blob/8c4ee76dab03402b6d5a89b08a2504939d6f23e4/lib/theming/src/create.ts#L175

this prevents the user from setting a brandImage

[hipstersmoothie]

Although it seems like the combination of these two PRs would solve my issues

https://github.com/storybooks/storybook/pull/6522
https://github.com/storybooks/storybook/pull/6476

Feel free to choose those over mine! It does about the same thing

[shilman]

Shiver me timbers!! I just released https://github.com/storybooks/storybook/releases/tag/v5.1.0-alpha.30 containing PR #6543 that references this issue. Upgrade today to try it out!

Because it's a pre-release you can find it on the @next NPM tag.

[shilman]

Ooh-la-la!! I just released https://github.com/storybooks/storybook/releases/tag/v5.0.10 containing PR #6543 that references this issue. Upgrade today to try it out!