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

[gatsby-plugin-manifest] Support SVG favicons #23923

Closed
idleberg opened this issue May 8, 2020 · 15 comments · Fixed by #25276
Closed

[gatsby-plugin-manifest] Support SVG favicons #23923

idleberg opened this issue May 8, 2020 · 15 comments · Fixed by #25276
Assignees
Labels
topic: plugins-PWA Issues related to PWA: the gatsby-plugin-offline and gatsby-plugin-manifest plugins type: feature or enhancement Issue that is not a bug and requests the addition of a new feature or enhancement.

Comments

@idleberg
Copy link

idleberg commented May 8, 2020

Summary

With modern browsers supporting SVG favicons and dark mode, I was experimenting using with the prefers-color-scheme media query inside an SVG file. While this works fine, gatsby-plugin-manifest currently provides no option to make use of this trick – all images are converted to PNG. A prefer_svg (or prefer_svg_favicon) option in gatsby-plugin-manifest would give users an option to go with that simple solution.

Motivation

To provide the optimal experience for users with visual impairments, serving an SVG with prefers-color-scheme media query can is a simple solution. I understand that performance is one of the driving factors of Gatsby, but not only can a single SVGs be smaller than a PNG. Any workaround to provide the same experience involves extra-code and providing two files.

@idleberg idleberg added the type: feature or enhancement Issue that is not a bug and requests the addition of a new feature or enhancement. label May 8, 2020
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 8, 2020
@ascorbic
Copy link
Contributor

This sounds like a useful enhancement. Any thoughts, @moonmeister? If anyone would like to contribute a PR that would be welcome.

@ascorbic ascorbic added help wanted Issue with a clear description that the community can help with. and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels May 11, 2020
@moonmeister
Copy link
Contributor

For clarity of discussion and implementation lets do a little mini-RFC discussion here and define what the functionality and config would look like. I'll start by reviewing current favicon support for Automatic and Hybrid modes(manual is unsupported) we can use the source icon (which currently supports SVG), to create the 48x48 PNG for favicon use.

What I think you're asking for is that the src icon (if it's svg) not be converted at all to png, simply passed on unmodified to be the favicon, is this correct? Would you expect an PNG src icon to be converted to SVG? (i'd assume no) Is there a way to fallback on browsers that don't support SVG favicon to a PNG favicon?

If all this is the case I don't think we'd need to create any API and can simply make this a progressive enhancement. We simply detect if the source icon is an SVG and pass it along to the favicon with the added support for the PNG fallback. This way we don't break any sites, the functionality becomes available with some documentation.

If you can confirm this would meet your expectations, and that we can fall back to a PNG favicon then this should be doable. If there's no fallback we'll have to do some more thinking and designing on how/if this can or should be done.

@moonmeister
Copy link
Contributor

Looks like fall-backs are easy, so if my described functionality is what you want @idleberg then this should be any easy implementation. https://catalin.red/svg-favicon-light-dark-theme/#browser-support-and-fallbacks

@idleberg
Copy link
Author

Would you expect an PNG src icon to be converted to SVG?

No, but maybe I'm not imaginative enough to think of a useful use-case

If all this is the case I don't think we'd need to create any API and can simply make this a progressive enhancement

Sounds good to me, especially with callbacks working

Overall, I'd like to thank you for the positive response! In the meantime, I've managed to implement a workaround in Helmet. But of course, simpler is better.

@moonmeister
Copy link
Contributor

It's not just you, I can't think of a useful use case for converting png either. 🙃

@danabrit danabrit added the topic: plugins-PWA Issues related to PWA: the gatsby-plugin-offline and gatsby-plugin-manifest plugins label May 21, 2020
@github-actions

This comment has been minimized.

@github-actions github-actions bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Jun 11, 2020
@moonmeister moonmeister added not stale and removed stale? Issue that may be closed soon due to the original author not responding any more. labels Jun 11, 2020
@janosh
Copy link
Contributor

janosh commented Jun 21, 2020

Great proposal!

Could someone outline what needs to be done? Just to get an idea how big a PR this would be.

@moonmeister
Copy link
Contributor

Mostly it'll be a matter of detecting if an SVG was specified (sharp should be able to help with this), if it is, add the primary SVG favicon code to head in addition to the fallback PNG. Then testing to verify all the work. So small/medium size PR.

@moonmeister moonmeister removed the help wanted Issue with a clear description that the community can help with. label Jun 25, 2020
@moonmeister
Copy link
Contributor

Started work on this. ^^

@RyanPinPB
Copy link

Was this feature implemented yet?

https://www.gatsbyjs.org/packages/gatsby-plugin-manifest/

I noticed the option "disable-favicon" talks about this (in above link). I'm still having an issue with the public folder receiving the SVG that was added to the src/images/favicon.svg with inline styles media queries for dark/light mode.

@moonmeister
Copy link
Contributor

moonmeister commented Jul 28, 2020

Should work. Check your public folder for "favicon.svg". Also this came out in the latest release, so make sure the plugin is up to date.

@RyanPinPB
Copy link

Needed to update to 2.4.21, the latest gatsby-source-wordpress-experimental post that I found and started from only required 2.4.12 or something like that.

Now my public folder is getting the SVG after updated the version

Thanks.

@RyanPinPB
Copy link

Now that I'm getting the .svg file copied to the public folder, I'm having issues with the backup PNG's replacing/overriding it.

Using Chrome so it should recognize the SVG.

Do I need to set "include_favicon: false" in the gatsby-config.js file in order to stop the pngs from being created and referenced as a backup?

This doesn't seem to be the intended use for "include favicon", is it?

after 'gatsby develop', if I delete all of the .png files that are created (for backups I guess?) from my .svg, the svg favicon works as planned. But upon the current build (if I don't manually go in and delete the PNGs), the PNG's will replace the SVG, not giving me the dark/light mode media queries I'm trying to get w/ my SVG.

@moonmeister
Copy link
Contributor

moonmeister commented Jul 28, 2020

Please open a new issue with a minimal reproducible test case and we'll see what we can do. It's up to the browser to pick the icon it thinks is best, chrome should pick the svg but maybe there's some requirement not met. Feel free to tag me there.

@LekoArts LekoArts removed the not stale label Sep 9, 2020
@grgcnnr
Copy link
Contributor

grgcnnr commented Sep 24, 2020

Here's a minimal repro:
https://github.com/grgcnnr/gatsby-plugin-manifest-Support-SVG-favicons-23923

And a new issue:
#27034

It appears that chromium based browsers (have tested in brave & chrome) continue to prefer the png option over the svg one.

adding rel='icon alternate' resolves this but i'm unsure if its semantically correct or has other side effects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: plugins-PWA Issues related to PWA: the gatsby-plugin-offline and gatsby-plugin-manifest plugins type: feature or enhancement Issue that is not a bug and requests the addition of a new feature or enhancement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants