Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: favicon, app icons and optional Web Manifest #1205
feat: favicon, app icons and optional Web Manifest #1205
Changes from 22 commits
bd181a3
50d3a5d
3b06651
0568ecb
4f6d011
50796b4
c9db5ed
de8a591
9ce807a
e1fbf88
6c48312
9ed466e
7dac4ce
5c1b756
286f8d1
4bc9214
990ba56
a299e7a
1ad46ea
b2af94a
ef7592b
6c36560
b514ff1
b3635dc
46debca
8e2bd5c
5efc854
dfd3538
d6fe8ae
26a94f0
81a662d
7c5b156
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found out why this image looks a bit more blurry than the others: it’s 140 pixels, not 180.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, the canvas should be 180px, but the image should be scaled to 140. I mixed that up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider compressing these images? A quick try at e.g. tinypng.com indicates we could save over 60% in file size for the PNG’s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I compressed them with the highest compression rate when exporting, but apparently much more was possible than that. I compressed them again with tinypng.com. Going from 3 and 9kB to a bit over 1 and 3kB respectively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also leave this file out if you remove it from the postinstall script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With Vincent we decided to split the favicon and web app stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you'll leave this file here to be reviewed in the webapp PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I was not thinking about splitting the PR, but only the documentation in this PR: one page about adding a (fav)icon to your website and one page about adding a web manifest and app icons to your web app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not following you I think. If you split the favicon and the webapp stuff I would leave out all the webapp stuff in this PR, for cleanliness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In hindsight, this should have been two PRs, but it seems we’re pretty close to merging. Splitting it up now feels like more work than gain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
app-icons
everywhere as we use a capital I inAppIcons
too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we split this page into two? The favicon should be required for all of our websites; the app icons are part of a PWA effort that requires more work anyway. It also makes each page a little easier to consume. Or is there a good reason against it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we can I think. Maybe better actually.
The only thing is the script that does all of it. If we want to keep the script I'll split that in two as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct that Android devices require these? If so, no app icon for them without a manifest? Please introduce this section with a sentence as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we're going to split the PWA/Web Manifest and favicon stuff we can move these icons over to the new PWA page. They are not used by webbrowsers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could go to the top of the Favicon page – the essential piece of HTML that people can just copy and paste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've swapped the Overview and Usage sections. It wouldn't make sense to move this up even higher, I think. People need some context.