Theme variable brandTitle incorrectly applied

[shilman]

Describe the bug
When we use the theme variable brandTitle on its own, it should replace the Storybook logo in the upper left with a string. Instead, it simply modifies the alt tag for the existing image.

Repro in official-storybook:

 theme: create({ brandTitle: 'foo', colorPrimary: 'hotpink', colorSecondary: 'orangered' }),

System:

• Version: 5.0.0-rc.11

[tmeasday]

The source of this issue is all these checks for null vs undefined in this component:

https://github.com/storybooks/storybook/blob/b19ae6b9e44e8759a4735169059cfa05f104d5e9/lib/ui/src/components/sidebar/SidebarHeading.js#L68-L93

Looking at the relevant stories, it looks like you are supposed to set the brandImage to null if you want no image:

https://github.com/storybooks/storybook/blob/b19ae6b9e44e8759a4735169059cfa05f104d5e9/lib/ui/src/components/sidebar/SidebarHeading.stories.js#L66-L79

@ndelangen / @domyen I'm not sure why the decision was to use null here rather than false, but in either case this is working as designed, what people are missing is they need to clear the image and set the text:

 theme: create({ brandTitle: 'foo', brandImage: null, colorPrimary: 'hotpink', colorSecondary: 'orangered' }),

A documentation issue?

[jjspace]

what people are missing is they need to clear the image and set the text

Just ran into this issue myself. I think the main problem is that the default Storybook logo/svg contains text so as a dev I assumed the brandTitle would replace the text Storybook and leave the image, the little pink book. To me the book seems like the brandImage and the text would be the brandTitle.

It's not clear from documentation or intuition that you can only have one OR the other, image OR text. I would recommend one of the following:

1. Documenting better
2. change the implementation to have a logo/image AND a text title with the book as the default brandImage and "Storybook" as the default brandTitle.

IMO the second option is more intuitive and easier to understand without having to dig/search through the Docs and would allow any devs to easily add their library's logo AND title without having to make an image/svg including the logo and text.

[ndelangen]

@jjspace I'll open a PR with the following change:

theme: create({ brandTitle: 'foo' }),

This will result in the text being shown, no need to set brandImage to null to achieve the effect.

What do you think?

[shilman]

Jeepers creepers!! I just released https://github.com/storybooks/storybook/releases/tag/v5.1.0-alpha.7 containing PR #6120 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.

Closing this issue. Please re-open if you think there's still more to do.

[shilman]

w00t!! I just released https://github.com/storybooks/storybook/releases/tag/v5.0.9 containing PR #6120 that references this issue. Upgrade today to try it out!

[shilman]

Yay!! I just released https://github.com/storybooks/storybook/releases/tag/v5.1.0-alpha.29 containing PR #6544 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]

If somebody can confirm #6444 I'll cherry pick it back into master and release a patch 🙏

[shilman]

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

[brendonco]

@shilman how to upgrade to https://github.com/storybooks/storybook/releases/tag/v5.1.0-alpha.29 ? will this be enough npx npm-check-updates '/storybook/' -u && npm install

[shilman]

We're on beta.0 now. I think npm-check-updates will work for prerelease versions -- try it? If it doesn't work, just edit your package.json file to set dependencies by hand.

[miguelyoobic95]

I am on 5.1.9 and I am still observing this issue - my brandTitle is ending up in the brandImage alt attribute of the image

[shilman]

@miguelyoobic95 are you specifying a custom brandImage? If so, I think that's the designed behavior. If not, maybe this regressed?

[ndelangen]

if you supply a custom Image AND a custom title, then the title will be placed as the alt-text on the image. This is intended behaviour. I'm unsure what else to expect.

[miguelyoobic95]

@shilman I am specifying a custom brandImage but I would like to add some text underneath, in this case the version number of the app. Is there anywhere to separate these two ? seems like a valid use case for me - a subtitle could be an alternative possibility maybe

[shilman]

@miguelyoobic95 I believe the intent is that you should include the text in your brandImage. cc @domyen who specified this feature.

[miguelyoobic95]

@shilman what if that text is dynamic i.e. a version number. I cannot include it as part of the brandImage (since I would have to regenerate the asset everytime) but I still think it would be valuable information to add as a subtitle for instance. Could you point me to where the brandImage code is ? I can help implement a subtitle if needed :) Happy to discuss our use case further but I think there is some value in either having both (image + title) or having a subtitle

[ndelangen]

@miguelyoobic95 after monoconfig is done, themes will gain the ability to have overriding react components.

Making your use-case possible.

But this is some time away. If you'd like, I could show you how far along monoconfig is, how it works, and possibly find a way for you to help?

[miguelyoobic95]

Hey @ndelangen sorry for the slow reply, we had some other stuff coming up. If you could give me some pointers to the config that would be great :)

[ndelangen]

Sure, join me on discord here:
https://discord.gg/sMFvFsG

And I'll talk you through what I've got so far.

[miguelyoobic95]

Hey @ndelangen, I actually found a workaround for our use case. It is a bit hacky but it works nicely and does exactly what we want it to do.

brandTitle: `<img src="./build/assets/logo/logo.svg" width="150px"/> YOBI Design\n\n ${appVersion}`

I am still happy to discuss on Discord towards the end of the week - I should have time this Friday so will contact you there. I have signed up for the group already.

Thanks again for all your time and help.

[karthiks]

I see this issue in current version of Storybook for React as well ("@storybook/react": "^5.3.18",).

@miguelyoobic95's hack works decently enough!!..

P.S: This issue is reproducible in this reference project: https://github.com/karthiks/build-a-burger

[rwieruch]

Hey @ndelangen, I actually found a workaround for our use case. It is a bit hacky but it works nicely and does exactly what we want it to do.

brandTitle: `<img src="./build/assets/logo/logo.svg" width="150px"/> YOBI Design\n\n ${appVersion}`

I am still happy to discuss on Discord towards the end of the week - I should have time this Friday so will contact you there. I have signed up for the group already.

Thanks again for all your time and help.

Thanks @miguelyoobic95 for this workaround. Indeed I ran into the same issue where I want to show a dynamic version number. I feel like Storybook shouldn't be opinionated about this. If I want to show a brandTitle AND brandImage, I should be allowed to do it.