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

Update favicon links in head #2804

Merged
merged 6 commits into from
Nov 26, 2024
Merged

Update favicon links in head #2804

merged 6 commits into from
Nov 26, 2024

Conversation

jayhesselberth
Copy link
Collaborator

Based on breaking changes from https://realfavicongenerator.net/

Fixes #2801

@jayhesselberth
Copy link
Collaborator Author

I think this is correct, based on this page.

Can someone please kick the tires?

Copy link

github-actions bot commented Oct 25, 2024

@hadley
Copy link
Member

hadley commented Oct 25, 2024

  1. Do we need to link to the .svg favicon too?
  2. Would we get a better svg favicon if we switch to a svg logo? (I'm not sure if there are other concerns with that)
  3. Did you have to delete the older files manually or does build_favicon() already handle that?

@jayhesselberth
Copy link
Collaborator Author

Maybe this is a good reference for what we need to include to "modernize" our approach.

@jayhesselberth
Copy link
Collaborator Author

jayhesselberth commented Oct 25, 2024

@hadley I added the svg icons in the head, seems like the modern way. I would imagine you'd get a better SVG favicon if you started with an SVG. And I deleted the favicons manually, build_favicons() doens't handle that.

Do you have an SVG version of the logo to test?

@jayhesselberth
Copy link
Collaborator Author

jayhesselberth commented Oct 27, 2024

The favicon checker reports some issues with the netlify deploy link (https://671c0d339cfbdfdb6da19660--pkgdown-dev.netlify.app/dev/)

There are issues with the site manifest, but more concerningly it says the SVG and ICO favicons are unavailable, despite being available and in the same place as the PNG favicon.

image

This might have something to do with a leading slash, which we do not include, but is included on the realfavicongenerator.net itself. Pasting realfavicongenerator.net into the checker, it looks like everything passes.

This could also be caused by the dev site URL, wondering if the paths on a dev site should be relative to "/dev".

@jayhesselberth
Copy link
Collaborator Author

Pretty sure the checker fails because the links need to be relative to /dev in this dev site, but can't figure out how to modify the href to include.

@jayhesselberth
Copy link
Collaborator Author

@hadley Do you think we will ever need the webmanifest file? It's only used if the site is used as a phone app (e.g., you make the site a button on a phone). If we want to keep, we'll need to modify the contents in place, which will be a little fiddly.

@hadley
Copy link
Member

hadley commented Nov 26, 2024

I think it's fine to leave as is, given that it only affects dev sites. I'm also not sure how favicons work with non-root sites now, because it used to be that favicon.ico only worked in the root directory.

@jayhesselberth
Copy link
Collaborator Author

Some of the tests are failing but I can't reproduce locally.

@jayhesselberth jayhesselberth merged commit dd777ee into main Nov 26, 2024
7 of 15 checks passed
@jayhesselberth jayhesselberth deleted the favicon-update branch November 26, 2024 15:09
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.

RealFavicon API now returns different set of favicons
2 participants