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

Density descriptors in manifest.json should not be included #228

Closed
marcoscaceres opened this issue Apr 8, 2016 · 8 comments
Closed

Density descriptors in manifest.json should not be included #228

marcoscaceres opened this issue Apr 8, 2016 · 8 comments
Labels
bug
Milestone

Comments

@marcoscaceres
Copy link

@marcoscaceres marcoscaceres commented Apr 8, 2016

Hi, I edit the W3C Manifest specification and work on Firefox's implementation of W3C manifest. Firstly, thanks for adding W3C manifest support via this plugin! ❤️ We see from our stats that many sites are including the manifest.

I would like to point out a small, but significant, issue that's currently affecting users. The usage of the "density" member in your generated manifest is currently being used incorrectly and should be removed.

The purpose of "density" is to say: "only ever use this icon when the density of the screen is at least X" (see spec). Right now, almost none of the icons will match, because there are no screen that, for instance, have a density "4.0" and very few that have "3.0".

I would kindly ask that you please remove the density member from the generated manifest.

Please let me know if you have any questions and if I can help fix this! I tried to track down the generator for this, but I had trouble finding it.

@marcoscaceres
Copy link
Author

@marcoscaceres marcoscaceres commented Apr 8, 2016

Oh, forgot to point out that "density": "1.0" is also implied - so you get that for free.

@kenchris
Copy link

@kenchris kenchris commented Apr 8, 2016

Actually on phones densities like 3.0 and 4.0 are kind of standard today. My second gen Moto X has a density of 3.0, but newer ones like the Galaxy S6/S7 are 4.0:

http://www.canbike.org/CSSpixels/

@marcoscaceres
Copy link
Author

@marcoscaceres marcoscaceres commented Apr 8, 2016

Ok, but that's not the point: a 192x192px icon is fine to use at 1x (even if designed for 4x). You don't want to discriminate on density unless you absolutely need to. It might make more sense for us to drop density from the spec and implementations if people using it incorrectly. Discriminating on density is a bit of a corner case, specifically nowadays where 2x is the norm and 3x is also becoming normal.

Sent from my iPhone

On 8 Apr 2016, at 6:46 PM, Kenneth Rohde Christiansen notifications@github.com wrote:

Actually on phones densities like 3.0 and 4.0 are kind of standard today. My second gen Moto X has a density of 3.0, but newer ones like the Galaxy S6/S7 are 4.0:

http://www.canbike.org/CSSpixels/


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@paulirish
Copy link

@paulirish paulirish commented Apr 12, 2016

Agree. I was surprised to see these in the output.

@phbernard
Copy link
Contributor

@phbernard phbernard commented Apr 13, 2016

Thank you @marcoscaceres for your feedback. I'm glad the manifest is spreading and I hope RFG is a significant contributor :)

Okay, I understand. I'm going to remove the density member. This change will be part of the next release, which should happen in a couple of weeks. Before, I need to update the RFG's compatibility test to make sure the package still works everywhere (although this density thing doesn't sound disruptive; but there are other changes to test, too).

FYI, I generated the manifest as described in the Android Chrome official doc. @paulirish I have no idea if that's up to you, but maybe you could give it a look?

@phbernard phbernard added the bug label Apr 13, 2016
@phbernard phbernard added this to the Package v0.13 milestone Apr 13, 2016
@kenchris
Copy link

@kenchris kenchris commented Apr 13, 2016

@PaulKinlan should be able to fix the docs, or know who can

@phbernard
Copy link
Contributor

@phbernard phbernard commented May 17, 2016

Implemented in branch package_0_13

@phbernard
Copy link
Contributor

@phbernard phbernard commented Jul 1, 2016

Deployed yesterday

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.