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.

Sign up for GitHub

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

Jump to bottom

feat: favicon, app icons and optional Web Manifest #1205

Merged
merged 32 commits into from
May 8, 2024
Merged

feat: favicon, app icons and optional Web Manifest #1205

merged 32 commits into from
May 8, 2024

Conversation

RubenSibon
Copy link
Contributor

No description provided.

@RubenSibon RubenSibon self-assigned this Apr 19, 2024
@github-actions github-actions bot temporarily deployed to demo-DES-652-favicon-and-app-icons April 19, 2024 10:57 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-652-favicon-and-app-icons April 19, 2024 11:34 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-652-favicon-and-app-icons April 19, 2024 11:46 Destroyed
@RubenSibon RubenSibon changed the title Favicon, app icons and optional Web Manifest feat: favicon, app icons and optional Web Manifest Apr 19, 2024
@github-actions github-actions bot temporarily deployed to demo-DES-652-favicon-and-app-icons April 19, 2024 16:07 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-652-favicon-and-app-icons April 19, 2024 16:51 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-652-favicon-and-app-icons April 19, 2024 16:56 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-652-favicon-and-app-icons April 24, 2024 15:30 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-652-favicon-and-app-icons April 24, 2024 15:36 Destroyed
VincentSmedinga
VincentSmedinga reviewed Apr 24, 2024
View reviewed changes
proprietary/assets/favicon/apple-touch-icon.png Outdated
Copy link
Contributor

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.

Copy link
Contributor Author

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.

dlnr
dlnr reviewed Apr 25, 2024
View reviewed changes
storybook/src/docs/app-icon.docs.mdx Outdated Show resolved Hide resolved
proprietary/assets/.postinstall.sh Outdated Show resolved Hide resolved
proprietary/assets/manifest/app.webmanifest
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

@github-actions github-actions bot temporarily deployed to demo-DES-652-favicon-and-app-icons May 2, 2024 15:48 Destroyed
@RubenSibon RubenSibon requested review from VincentSmedinga and dlnr May 2, 2024 16:08
dlnr
dlnr reviewed May 3, 2024
View reviewed changes
proprietary/assets/manifest/app.webmanifest
Copy link
Contributor

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?

proprietary/assets/favicon/icon.svg Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
storybook/src/docs/web-app.docs.mdx Outdated Show resolved Hide resolved
@dlnr dlnr self-requested a review May 3, 2024 18:19
@RubenSibon @dlnr
style(svg): Remove clipping path
d6fe8ae 
Co-authored-by: Niels Roozemond <[email protected]>
@github-actions github-actions bot temporarily deployed to demo-DES-652-favicon-and-app-icons May 6, 2024 09:31 Destroyed
RubenSibon
RubenSibon commented May 6, 2024
View reviewed changes
package.json Outdated Show resolved Hide resolved
@Amsterdam Amsterdam deleted a comment from dlnr May 6, 2024
@github-actions github-actions bot temporarily deployed to demo-DES-652-favicon-and-app-icons May 6, 2024 09:36 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-652-favicon-and-app-icons May 7, 2024 14:32 Destroyed
VincentSmedinga
VincentSmedinga approved these changes May 7, 2024
View reviewed changes
Copy link
Contributor

@VincentSmedinga VincentSmedinga left a comment

Choose a reason for hiding this comment

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

Thanks for all this work and a good result. Next time, take it easy with the scope creep - the web manifest stuff is nice but wasn’t requested and slowed us down a bit :)

proprietary/assets/favicon/apple-touch-icon.png Outdated Show resolved Hide resolved
proprietary/assets/manifest/app.webmanifest
Copy link
Contributor

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.

@github-actions github-actions bot temporarily deployed to demo-DES-652-favicon-and-app-icons May 8, 2024 16:13 Destroyed
@RubenSibon RubenSibon merged commit 5513961 into develop May 8, 2024
4 checks passed
@RubenSibon RubenSibon deleted the feat/DES-652-favicon-and-app-icons branch May 8, 2024 16:16
@github-actions github-actions bot mentioned this pull request Jun 5, 2024
Merged
@github-actions github-actions bot mentioned this pull request Oct 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VincentSmedinga VincentSmedinga VincentSmedinga approved these changes

@alimpens alimpens Awaiting requested review from alimpens

@dlnr dlnr Awaiting requested review from dlnr

Assignees

@RubenSibon RubenSibon

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RubenSibon @VincentSmedinga @dlnr