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

Geolocation audit now returns unknown if page already had granted permission #925

Merged
merged 4 commits into from
Nov 13, 2016

Conversation

ebidel
Copy link
Contributor

@ebidel ebidel commented Nov 9, 2016

R: @brendankenny @paulirish @GoogleChrome/lighthouse

This also adds debugString support to the audit if it specifies one:

screen shot 2016-11-08 at 4 49 36 pm

@ebidel
Copy link
Contributor Author

ebidel commented Nov 9, 2016

Maybe it's too much? Don't want to spend a ton of time on it though since the report is being redone.

screen shot 2016-11-08 at 4 53 55 pm

@ebidel ebidel changed the title Geolocation audit now returns unknown if page already had granted permision Geolocation audit now returns unknown if page already had granted permission Nov 9, 2016
@ebidel
Copy link
Contributor Author

ebidel commented Nov 9, 2016

The debug boxes look like 💩 . I'm not 😄 with them. Let me tweak a bit, but you guys can look over the code changes.

@paulirish
Copy link
Member

@ebidel actually @chowse is updating the report looks right now so you can hold off. :)

@ebidel
Copy link
Contributor Author

ebidel commented Nov 9, 2016

Sounds good. Can the new stuff be sure to do something with the .debugString?

@ebidel
Copy link
Contributor Author

ebidel commented Nov 9, 2016

Slight update:

screen shot 2016-11-09 at 10 01 18 am

I know #926 is in the works so I'll roll out the style changes.

@ebidel
Copy link
Contributor Author

ebidel commented Nov 9, 2016

@paulirish removed the report changes. Ready.

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.

a few questions but nothing blocking.


/* istanbul ignore next */
function queryGeolocationPermission() {
return navigator.permissions.query({name: 'geolocation'}).then(result => {
Copy link
Member

Choose a reason for hiding this comment

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

thats very cool. do we need to do a feature detect before using this or are we safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

safe. chrome has had the permission api since 43

@@ -33,6 +33,17 @@ describe('UX: geolocation audit', () => {
assert.ok(auditResult.debugString);
});

it('prints debugString information when present', () => {
Copy link
Member

Choose a reason for hiding this comment

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

"prints debugString info if present in artifact"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@paulirish paulirish merged commit 5776ed9 into master Nov 13, 2016
@paulirish paulirish deleted the geo2 branch November 13, 2016 00:04
andrewrota pushed a commit to andrewrota/lighthouse that referenced this pull request Jan 13, 2017
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.

2 participants