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

Add "minimal-ui" as an allowed display value. #1268

Merged
merged 3 commits into from
Jan 3, 2017

Conversation

dracos
Copy link
Contributor

@dracos dracos commented Dec 23, 2016

Commit 98c4980 said "manifest display must be one of the 3 allowed values.", and this adds the fourth allowed value. For #495.

@dracos dracos changed the title Add "minimal-ui" is an allowed display value. Add "minimal-ui" as an allowed display value. Dec 23, 2016
Commit 98c4980 said "manifest display must be one of the 3 allowed
values.", and this adds the fourth allowed value.
@wardpeet
Copy link
Collaborator

Could you add the value to the test-case as well
lighthouse-core/test/audits/display-test.js

@dracos
Copy link
Contributor Author

dracos commented Dec 26, 2016

Done, sorry for missing.

@@ -37,7 +37,8 @@ class ManifestDisplay extends Audit {
* @return {boolean}
*/
static hasRecommendedValue(val) {
return (val === 'fullscreen' || val === 'standalone' || val === 'browser');
return (val === 'fullscreen' || val === 'standalone' ||
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be: ['browser', 'fullscreen', 'minimal-ui', 'standalone'].indexOf(val) !== -1;

Copy link
Member

Choose a reason for hiding this comment

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

+1. Let's do that.

@ebidel
Copy link
Contributor

ebidel commented Dec 30, 2016

Giving this to @paulirish for comment.

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

sg.

approved modulo the style tweak that eric suggested. I'll made that change myself to make it easier. :)

@@ -37,7 +37,8 @@ class ManifestDisplay extends Audit {
* @return {boolean}
*/
static hasRecommendedValue(val) {
return (val === 'fullscreen' || val === 'standalone' || val === 'browser');
return (val === 'fullscreen' || val === 'standalone' ||
Copy link
Member

Choose a reason for hiding this comment

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

+1. Let's do that.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@paulirish paulirish merged commit 6edd84e into GoogleChrome:master Jan 3, 2017
@brendankenny
Copy link
Member

brendankenny commented Jan 3, 2017

Wait, so what is the purpose of this audit now? This passes on all possible values of display (because unknown and missing values are defaulted to browser)

@paulirish
Copy link
Member

This audit is in "Other" and not part of the PWA test. It's a soft audit on correctness of your manifest.

We haven't been using a "standalone" audit for PWA-ness for a while now.

@brendankenny
Copy link
Member

Right, it was testing poorly before, so I guess you could argue this at least makes it do nothing consistently :)

I just mean the only way to fail this audit now is to not have a manifest at all. Do we still need it?

andrewrota pushed a commit to andrewrota/lighthouse that referenced this pull request Jan 13, 2017
* Add "minimal-ui" as an allowed display value.

Commit 98c4980 said "manifest display must be one of the 3 allowed
values.", and this adds the fourth allowed value.

* Adopt style suggested by Eric
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.

6 participants