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

Fix usage of custom favicon #592

Merged
merged 2 commits into from
Nov 3, 2016
Merged

Conversation

jeffcarbs
Copy link
Contributor

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 to app.use(favicon) wins, so we need to do the default last.

@thani-sh
Copy link
Contributor

thani-sh commented Nov 2, 2016

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
Copy link
Member

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.

Copy link
Contributor Author

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.

@arunoda
Copy link
Member

arunoda commented Nov 2, 2016

@jeffcarbs
Copy link
Contributor Author

@arunoda - Looks like that line just copies the favicon from /public/favicon.ico into the main outputDir directory. If you define a static dir, all those assets will then be copied into that same directory afterward (https://github.com/kadirahq/react-storybook/blob/8610bca7b0cbba93fa6c770dad4404325be6e549/src/server/build.js#L65-L74) so if you've defined a favicon.ico it will override the default.

Given that, it looks like this is only an issue in development which is using express. When the build is compiled statically your custom favicon.ico will be the one that gets built.

@jeffcarbs
Copy link
Contributor Author

Updated! I built and tested this in my app and it works as expected.

@arunoda
Copy link
Member

arunoda commented Nov 3, 2016

@jcarbo yep. You've a solid point in that.

@arunoda arunoda merged commit 2659293 into storybookjs:master Nov 3, 2016
@jeffcarbs jeffcarbs deleted the feature/favicon branch November 3, 2016 16:29
@shilman shilman added the misc label May 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants