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 28 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
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.
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.