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

optimize how we set the shortcut icon #3621

Merged
merged 7 commits into from
Apr 29, 2021

Conversation

peterbe
Copy link
Contributor

@peterbe peterbe commented Apr 23, 2021

Part of #3611

Now, after the react-scripts has created the client/build/ with all the magic that create-react-app + webpack does, another script kicks in and "picks up the slack".
At the moment, the only thing it does is to post-process (aka. optimize) the <link rel="..." href="..."> tags' href value.
I.e.

<link rel="shortcut icon" href="/favicon.ico" type="image/x-icon"/>

...becomes...

<link rel="shortcut icon" href="/favicon.323ad90c.ico" type="image/x-icon"/>

and it also copies the file client/build/favicon.ico to client/build/favicon.323ad90c.ico.

Now the Deployer will serve the URL https://developer.m.o/favicon.323ad90c.ico with a very aggressive Cache-Control which is safe to do because it's md5 hashed by the file itself.

@peterbe peterbe requested a review from Gregoor April 23, 2021 18:45
@peterbe
Copy link
Contributor Author

peterbe commented Apr 23, 2021

I pushed a complete build of this branch to Dev.
E.g. https://optimized-favicons.content.dev.mdn.mozit.cloud/en-US/docs/web/Javascript
Look at the web console (Network panel). Seems to work!

CC @escattone

Copy link

@myakura myakura left a comment

Choose a reason for hiding this comment

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

If MDN doesn't need to (or no longer) support IE10 or lower, you can omit the shortcut part and the type attribute. <link rel="icon" href="path"> works across modern browsers (including IE11).

@myakura
Copy link

myakura commented Apr 26, 2021

Does MDN still need to serve an ICO file for the favicon? If it doesn't need to support IE10 or lower, you can use PNG and that should cut down the bytes even further.

@peterbe
Copy link
Contributor Author

peterbe commented Apr 27, 2021

Does MDN still need to serve an ICO file for the favicon? If it doesn't need to support IE10 or lower, you can use PNG and that should cut down the bytes even further.

So if we use <link rel="icon" href="path"> it'll work on all modern browsers, but the old ones might still fall back to the convention URL of /favicon.ico which we're not going to remove. Sounds great!

@myakura
Copy link

myakura commented Apr 27, 2021

Here's a 48x48 PNG favicon, extracted from the current MDN favicon.ico using imagemagick:

mdn-favicon-48x48.png (835 bytes)

And this is an optimized one, reducing color to 16 colors using Squoosh:

mdn-favicon-48x48-optimized.png (373 bytes)

835 bytes → 373 bytes.

@peterbe
Copy link
Contributor Author

peterbe commented Apr 27, 2021

I think I'd rather use that when the HTML can be used and my favicon.ico only to be used as a fallback.

@peterbe peterbe marked this pull request as draft April 27, 2021 17:40
@peterbe
Copy link
Contributor Author

peterbe commented Apr 27, 2021

In about 1h https://optimized-favicons.content.dev.mdn.mozit.cloud/en-US/docs/web/Javascript should be working with the new update.

@peterbe peterbe marked this pull request as ready for review April 27, 2021 19:24
@peterbe
Copy link
Contributor Author

peterbe commented Apr 27, 2021

@myakura Can you take look now? The Dev build isn't ready at the time of writing but once it is, I think we can check.

@myakura
Copy link

myakura commented Apr 28, 2021

@peterbe I just checked with the preview and it works!

screenshot: dev build page shown in Firefox with devtools open

screenshot: dev build page shown in Chrome with devtools open

Both grab the 373 byte PNG icon which has longer cache TTL than what the current favicon does.

Checking out in IE11 was a bit tricky since the favicon request doesn't show up on devtools. But looking at the favicon cache, it seems to grab a 373 byte file so I believe it works on IE11 as well. Hope that helps.

C:\Users\myaku\Favorites>C:\Users\myaku\tools\Streams\streams.exe *

streams v1.60 - Reveal NTFS alternate streams.
Copyright (C) 2005-2016 Mark Russinovich
Sysinternals - www.sysinternals.com

C:\Users\myaku\Favorites\MDN Web Docs Dev.url:
         :favicon:$DATA 373
C:\Users\myaku\Favorites\MDN Web Docs.url:
         :favicon:$DATA 15086

Copy link
Contributor

@Gregoor Gregoor left a comment

Choose a reason for hiding this comment

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

Great stuff!

@peterbe peterbe merged commit 7fc6698 into main Apr 29, 2021
@peterbe peterbe deleted the 3611-optimize-how-we-set-the-shortcut-icon branch April 29, 2021 12:24
peterbe added a commit to peterbe/yari that referenced this pull request Jun 1, 2021
* optimize how we set the shortcut icon

Part of mdn#3611

* fix eslint problem

* fix scripts

* use logger.info instead

* add favicon-48x48.png for the new default favicon

* add a test
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

3 participants