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

Add favicon #70

Merged
merged 1 commit into from
Jul 22, 2016
Merged

Add favicon #70

merged 1 commit into from
Jul 22, 2016

Conversation

keyz
Copy link
Contributor

@keyz keyz commented Jul 22, 2016

Implements #69. The favicon file is taken from https://facebook.github.io/react/.

@ghost ghost added the CLA Signed label Jul 22, 2016
@gaearon gaearon merged commit cbb4123 into facebook:master Jul 22, 2016
@gaearon
Copy link
Contributor

gaearon commented Jul 22, 2016

👍

@gaearon gaearon deleted the add-favicon branch July 22, 2016 03:29
@@ -91,6 +91,7 @@ module.exports = {
new HtmlWebpackPlugin({
inject: true,
template: indexHtmlPath,
favicon: path.join(srcPath, 'favicon.ico'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work if it's in the root folder with HTML instead? This feels more like meta information than app module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would be better to have a folder that contains static assets (favicon.ico and logo.svg)? I think favicon.ico should be in that folder (let's call it assets?) but not sure about logo.svg.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yeah, it works when it's not in the src folder.

Copy link
Contributor

@lacker lacker Jul 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For logo.svg and similar images, since it gets imported from javascript, it's annoying to put it in a different root folder. You would end up with import '../../../../../../../../assets/foo/bar/baz/qux.png' in big projects rather than import './blah.png'. The favicon is different since webpack handles it without js doing anything.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. I moved favicon.ico to top level since it’s a “magic” file like index.html.
The modules live in src.

@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants