-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Fix usage of custom favicon #592
Conversation
9d14e08
to
b248f6e
Compare
Awesome! I'll just check it out locally in a few hours and merge |
}); | ||
} | ||
|
||
// The first call to app.use(favicon) wins so we first check if there's a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something documented?
I mean it's better to have a variable called hasCustomFavicon
and act accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just something I noticed, but you're right that it's not super clear. I'll update this to use hasCustomFavicon
based on your feedback.
@jcarbo I think we need to do the same in the |
@arunoda - Looks like that line just copies the favicon from Given that, it looks like this is only an issue in development which is using express. When the build is compiled statically your custom |
Updated! I built and tested this in my app and it works as expected. |
@jcarbo yep. You've a solid point in that. |
Fixes #581
This line (https://github.com/kadirahq/react-storybook/blob/master/src/server/index.js#L69) hard-codes the favicon to be pulled from
/node_modules/@kadira/storybook/dist/server/public/favicon.ico
.This update first checks the specified static files for a
favicon.ico
and uses that if one is found. Otherwise, it falls back to using the default storybook favicon. The tricky part here is that it seems that the first call toapp.use(favicon)
wins, so we need to do the default last.