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(gatsby-plugin-manifest): favicon path respects hybrid mode #8315

Merged
merged 6 commits into from
Sep 24, 2018

Conversation

zauni
Copy link
Contributor

@zauni zauni commented Sep 19, 2018

In hybrid mode, you can have a different path for the icons. This path is used when the icons get generated, but the favicon <link> tag is not using it. This fixes it.

@pieh
Copy link
Contributor

pieh commented Sep 19, 2018

Could we move

manifest.icons[0].src.substring(0, manifest.icons[0].src.lastIndexOf(`/`))
to something like utils file, so implementation is not duplicated in 2 (or more files)?

@zauni
Copy link
Contributor Author

zauni commented Sep 19, 2018

You are right! And I remembered that Node already has a function to get the folder of a path: dirname

But is it possible to require the path module from node in gatsby-ssr.js?

@zauni
Copy link
Contributor Author

zauni commented Sep 20, 2018

Hm... I realised now that not only the folder could be different, but also the filename, so I have to change that also. I will make a new commit later to account for that.

@pieh pieh changed the title [gatsby-plugin-manifest] Favicon path respects hybrid mode fix(gatsby-plugin-manifest): favicon path respects hybrid mode Sep 24, 2018
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Thanks @zauni!

@pieh pieh merged commit 05e7ccd into gatsbyjs:master Sep 24, 2018
oorestisime pushed a commit to oorestisime/gatsby that referenced this pull request Sep 28, 2018
@zauni zauni deleted the plugin-manifest-favicon-path-fix branch January 8, 2019 09:49
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.

None yet

2 participants