Fix 'SidebarHeading' according to its inputs

[lonyele]

Issue: #5866

When creating theme object, brandTitle, brandUrl, brandImage are optional but It is partially working as intended.(Cases such as defining brandTitle doesn't change the title)

How to reproduce

• Create theme object with defining only brandTitle. I expect sidebar's heading to be changed but It doesn't.

import { create } from '@storybook/theming';
const myTheme = create({
  base: 'light',
  brandTitle: 'My custom storybook',
});

• When brandUrl is only defined and with null , It should show the default storybook logo, but text 'Storybook' is shown

import { create } from '@storybook/theming';
const myTheme = create({
  base: 'light',
  brandUrl: null
});

What I did

I changed the logic of using theme's brand object to match all the possible cases(combinations of value being defined, undefined, null on brandTitle, brandUrl, brandImage)

How to test

• Is this testable with Jest or Chromatic screenshots? I'm not sure.
• Does this need a new example in the kitchen sink apps? No
• Does this need an update to the documentation? No

About the test can you please guide me if it is needed or how to do it? Maybe here? But the problem here is not when creating theme object, rather when it is before rendered. Then It sounds like an snapshot or e2e tests, as I've tried to find some tests on this I couldn't find it. Here?

[vercel]

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-fork-lonyele-fix-usageof-brand-object.storybook.now.sh

[shilman]

@lonyele thanks for this PR! If possible, can you search through the open theming issues and see if it resolves any open issue? If so, can you list them on top of this PR like:

Issue: #xxx #yyy

It will help us review the PR and also will notify people on the issues when we release this and they can test it out.

Thanks!

Cc @ndelangen

[lonyele]

@shilman Oh sorry for not searching issue first. I found this behaviour while migrating some of the code at storybook-readme and I just digged into the problem.

Luckily I found the exact same issue (#5866) and the following PR that tried to fix this. (#6120)

[ndelangen]

Didn't I fix this before?, I'm having some deja-vue..

So I'm confused.. does this fix an issue already fixed? Did my fix not work?

[ndelangen]

@lonyele could you tell me if my fix was actually broken, or you jump happened to find an alternative solution?

[lonyele]

@ndelangen Sorry for the late response(finally saturday here), I jumped to the solution because It didn't work when I used it at that time with any released version. I now freshly made a new project and still It doesn't work with v5.0.6 I found that the PR(#6120) that fixes this is on next branch and not yet released.

I applied the fixing PR and It works for all the cases! So... I should have checked the pre-release version. kind of stupid.(hm... this is strange... I've been on next branch)

I think only reasonable excuse for this PR to be reviewed is when a brand object is needed to be purely reflected to what user input. I feel like current solution arbitrarily tweaks the value of brandImage only to be used at SidebarHeading.js, however I think leaving this brand object as what user input is better in long run when this data is needed other places in the future. Just a suggestion from a newbie programmer.

[ndelangen]

Thanks for the contribution @lonyele

I'm closing this PR, because the issue you've addressed was already fixed in another PR.

Appreciate your time and energy, hope to see more PRs from you ❤️