Skip to content

Fixes #2771 - Removes unnecessary favicons#2804

Merged
miketaylr merged 1 commit intowebcompat:masterfrom
marimeireles:issues/2771/1
Feb 22, 2019
Merged

Fixes #2771 - Removes unnecessary favicons#2804
miketaylr merged 1 commit intowebcompat:masterfrom
marimeireles:issues/2771/1

Conversation

@marimeireles
Copy link
Copy Markdown
Member

Removed a few favicons that weren't necessary and were being downloaded when user was on Safari.

r? @miketaylr

@miketaylr
Copy link
Copy Markdown
Member

I want to be sure we're doing this for a principled reason and not just because there's a lot of them. @marimeireles do we have measurements of the before/after to justify deleting them, I guess in Safari since that's the browser with the bug?

@miketaylr
Copy link
Copy Markdown
Member

That said, I do agree that we're maintaining a lot of images just for the possibility that someone bookmarks the site on their phone or ipad or whatever, and wants a pretty icon.

@marimeireles
Copy link
Copy Markdown
Member Author

I want to be sure we're doing this for a principled reason and not just because there's a lot of them

No, but I can do it. I'll post my results in the issue #2771, better than here, right?

@miketaylr
Copy link
Copy Markdown
Member

@marimeireles can we do a little bit of research and find the minimal viable set of favicon images required to not look like crap on regular and hi-dpi monitors? I'm inclined to suggest we just burn it all down and revisit app icons once we add some app-like functionality (via service workers, or anything compelling that would inspire you to save the site to your home screen)

@marimeireles
Copy link
Copy Markdown
Member Author

I'm inclined to suggest we just burn it all down and revisit app icons once we add some app-like functionality (via service workers, or anything compelling that would inspire you to save the site to your home screen)

I thought we already had the functionality.

@miketaylr
Copy link
Copy Markdown
Member

I thought we already had the functionality.

No, we just have an app manifest. No service workers.

Copy link
Copy Markdown
Member

@miketaylr miketaylr left a comment

Choose a reason for hiding this comment

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

Thanks @marimeireles.

Any objections @magsout @karlcow?

@karlcow
Copy link
Copy Markdown
Member

karlcow commented Feb 20, 2019

no issue for me.

Copy link
Copy Markdown
Member

@miketaylr miketaylr left a comment

Choose a reason for hiding this comment

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

Cool, let's go for it. We can always change back if we decide it's important in the future.

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.

3 participants