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

Link to brandImage not working #9443

Closed
SuneRadich opened this issue Jan 14, 2020 · 20 comments
Closed

Link to brandImage not working #9443

SuneRadich opened this issue Jan 14, 2020 · 20 comments

Comments

@SuneRadich
Copy link

Describe the bug
During upgrade from 5.2.8 to 5.3.3 my brandImage stopped working.

I have a custom theme, using a static assets folder for the image

./storybook
 - ./public
   - logo.svg

Previously I had me theme defined like this:

import { create } from '@storybook/theming';
import logo from './public/logo.svg';

export default create({
  base: 'light',

  brandImage: logo,
  brandTitle: 'Custom - Storybook'
});

After updating to 5.3.3 I've moved my theming to manager.js, like so

import { addons } from '@storybook/addons';
import { create } from '@storybook/theming/create';
import logo from './public/logo.svg';

const theme = create({
  base: 'light',

  brandImage: `/${logo}`,
  brandTitle: 'Custom - Storybook'
});

addons.setConfig({
  panelPosition: 'bottom',
  theme
});

But the logo.svg does not show up when I start storybook using start-storybook -p 6006 -s ./.storybook/public.

If I however do a static build via build-storybook -s ./.storybook/public, the logo shows up correctly.

Webserver fetches the logo from /media/static/logo.svg in both cases. But it seems the local webserver started when starting storybook locally does not correctly allow fetching images from this folder.

System:
Environment Info:

System:
OS: macOS 10.15.2
CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
Binaries:
Node: 13.6.0 - ~/.nvm/versions/node/v13.6.0/bin/node
Yarn: 1.19.1 - /usr/local/bin/yarn
npm: 6.13.4 - ~/.nvm/versions/node/v13.6.0/bin/npm
Browsers:
Chrome: 79.0.3945.117
Safari: 13.0.4
npmPackages:
@storybook/addon-a11y: ^5.3.3 => 5.3.3
@storybook/addon-actions: ^5.3.3 => 5.3.3
@storybook/addon-docs: ^5.3.3 => 5.3.3
@storybook/addon-knobs: ^5.3.3 => 5.3.3
@storybook/addon-links: ^5.3.3 => 5.3.3
@storybook/addon-notes: ^5.3.3 => 5.3.3
@storybook/addon-viewport: ^5.3.3 => 5.3.3
@storybook/addons: ^5.3.3 => 5.3.3
@storybook/angular: ^5.3.3 => 5.3.3

@steven-pribilinskiy
Copy link

steven-pribilinskiy commented Jan 15, 2020

A temporary solution until it's fixed:

import logoUrl from './public/logo.svg';

const theme = create({
  brandImage: (process.env.NODE_ENV === 'production') ? logoUrl : '/logo.svg',
build-storybook
start-storybook -p 6006 -s .storybook/public

@chris-pearce
Copy link

Experiencing this issue too.

@shilman
Copy link
Member

shilman commented Jan 17, 2020

cc @ndelangen i think this is about moving from preview => manager

@ndelangen
Copy link
Member

Added to my todo-list

@ndelangen ndelangen self-assigned this Jan 23, 2020
@ndelangen
Copy link
Member

My findings so far:

First I though maybe there's a loader missing for the manager
not it, the loader is there

Then I thought the file-loader might not be aware of the real publicPath or something
not it, it's working as intended, no further config would help I think

Now I'm thinking that in the express app, the routers for preview are preferred over the routes of the manager. The preview returns a 404, even though the manager has a route for the image. There are 2 webpack's being served, and being combined in express.
investigating this option

@ndelangen
Copy link
Member

ndelangen commented Jan 27, 2020

I have a PR open with a fix

If you're able I'd love a review @SuneRadich

@jcousins-ynap
Copy link

I'm finding that the brand image URL is being defined, but locally it can't find static/media/brandImage.filename.png but when built the image exists in that location.

@ndelangen
Copy link
Member

@jcousins-ynap yes, my PR fixes that

@jcousins-ynap
Copy link

Nicely done @ndelangen much appreciated.

@daphen
Copy link

daphen commented Feb 6, 2020

Still waiting for this? He doesn't seem to be very active.

@shilman
Copy link
Member

shilman commented Feb 7, 2020

@daphen he's on it. please chill.

@daphen
Copy link

daphen commented Feb 7, 2020

@shilman Sorry, I didn't mean to come off as aggrevated. 😅 If he's on it then it's just a matter of waiting. I was under the impression that he wasn't here.

@shilman
Copy link
Member

shilman commented Feb 7, 2020

Jeepers creepers!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.0.0-alpha.7 containing PR #9646 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

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

@shilman shilman closed this as completed Feb 7, 2020
@cone56
Copy link
Contributor

cone56 commented Feb 20, 2020

@shilman would it be possible to backport the fix to the 5.3.x branch and republish? It's in the 5.3 hotlist, but it seems it was mistakenly merged to a prerelease branch. I'm still unable to update storybook because of this bug.

@ndelangen
Copy link
Member

@cone56 it might be possible, but the change to make this work isn't trivial.. I agree it would likely be possible and is desired, but I'd have to find the time to backport it and test.

@shilman
Copy link
Member

shilman commented Feb 20, 2020

@cone56 Not a mistake: we typically merge everything into next, release on an alpha, and then patch bugfixes back to master when they are both high priority and low risk.

I'd say this one is fairly high priority but medium risk. If you'd like to help get the fix backported, can you test it out on the 6.0-alpha release and confirm that it's working well for you? Then @ndelangen and I can figure out whether we're comfortable doing the patch. Thanks!

@cone56
Copy link
Contributor

cone56 commented Feb 22, 2020

@shilman @ndelangen I've just tested the latest alpha release and can confirm the fix is working well. If there's anything else I can help test, please let me know.

Thanks also for explaining your release process; I completely understand that these things are not as easy as they seem on the surface and can take time.

@shilman
Copy link
Member

shilman commented Feb 22, 2020

Thanks for testing @cone56! I'll see what we can do this week.

@kylepeeler
Copy link

Anyway to get this fixed pushed to 5.3? The workaround requires making a static build :(

@shilman
Copy link
Member

shilman commented Mar 31, 2020

Whoopee!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.3.18 containing PR #9646 that references this issue. Upgrade today to try it out!

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

No branches or pull requests

9 participants