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

[fix bug 1045961] Add larger icons to homepage #2331

Merged
merged 1 commit into from Oct 8, 2014

Conversation

ckprice
Copy link

@ckprice ckprice commented Oct 1, 2014

Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1045961

Created new icons for mobile "Add to homescreen" integration.

@alexgibson
Copy link
Member

Thanks @ckprice!

Reading back through the bug, I think we can simplify what you have here a bit.

For iOS, I think we're safe to include a single apple-touch-icon (the largest variant) as the device will scale down and resize automatically. I believe the largest size for iOS is also now 180x180.

Secondly, because of the Firefox OS bug whereby the last icon declared is always chosen (irrespective of size), we could either consider reversing the order of rel="icon" so it goes from smallest to largest. Or alternatively we could just serve a single 196x196 icon? What do you think?

Note: you will also need to make this change to bedrock/base/templates/base.html

Thanks!

@@ -34,6 +34,8 @@
<meta name="twitter:app:name:googleplay" content="{% block android_app_name %}{{ _('Firefox') }}{% endblock %}">
<meta name="twitter:app:id:googleplay" content="{% block android_app_id %}org.mozilla.firefox{% endblock %}">
<link rel="shortcut icon" href="{% block page_favicon %}{{ media('img/favicon.ico') }}{% endblock %}">
<link rel="apple-touch-icon" sizes="180x180" href="{{ media('img/favicon/apple-touch-icon-180x180.png') }}">
<link rel="icon" type="image/png" href="{{ media('img/favicon/favicon-196x196.png') }}" sizes="196x196">
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can we move the sizes attribute here to immediately before the href, so that it's more consistent with the apple-touch-icon ordering? Same applies to base.html

@alexgibson
Copy link
Member

If you run both these new icons through imageoptim we can save around 80% on the file size.

@alexgibson
Copy link
Member

Tested this on iOS and Firefox OS 2.2.0 and I see appropriately sized icons for each when added to home screen 👍

It's an r+wc from me - just a couple of things to tidy up, then if you can squash commits I think we're good to merge this.

@ckprice ckprice force-pushed the bug-1045961-homescreen-icons branch from daa3c6c to 4a2a2a4 Compare October 7, 2014 16:11
@ckprice
Copy link
Author

ckprice commented Oct 7, 2014

Thanks @alexgibson! Updates made.

@alexgibson
Copy link
Member

r+

alexgibson added a commit that referenced this pull request Oct 8, 2014
[fix bug 1045961] Add larger icons to homepage
@alexgibson alexgibson merged commit 154bcfd into mozilla:master Oct 8, 2014
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

2 participants