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.

Sign up for GitHub

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 permissionStatus property #32

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add permissionStatus property #32

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c7cd21b
Add `permissionStatus` property
FluorescentHallucinogen Dec 19, 2017
292ddf0
'Gelocation' -> 'Geolocation'
FluorescentHallucinogen Dec 19, 2017
862b9cd
Remove "permission" keyword
FluorescentHallucinogen Feb 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

Geolocation API Polymer web component.

Example to get the device geolocation values:
Get once and display the device geolocation values and continuous update the Geolocation API permission status:
<!---
```
<custom-element-demo>
Expand All @@ -23,9 +23,10 @@ Example to get the device geolocation values:
```
-->
```html
<geo-location latitude="{{latitude}}" longitude="{{longitude}}"></geo-location>
<geo-location permission-status="{{permissionStatus}}" latitude="{{latitude}}" longitude="{{longitude}}"></geo-location>

<ul>
<li>Permission status: [[permissionStatus]]</li>
<li>Latitude: [[latitude]]</li>
<li>Longitude: [[longitude]]</li>
</ul>
Expand Down
2 changes: 2 additions & 0 deletions bower.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
"location",
"position",
"navigation",
"api",
Copy link
Owner

@ebidel ebidel Dec 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

don't think we need these. they're vague

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ebidel I've removed the permission keyword. But IMO we should keep the api keyword. <geo-location> web component is a part of my progressive-elements collection that is a set of web components for modern web APIs such as Payment Request API, Media Session API, etc. People can find these web components by the api keyword.

"permission",
"coordinates",
"latitude",
"longitude",
Expand Down
5 changes: 3 additions & 2 deletions demo/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,15 @@
</head>
<body>
<div class="vertical-section-container centered">
<h3>Get the device geolocation values</h3>
<h3>Get once and display the device geolocation values and continuous update the Geolocation API permission status</h3>
<demo-snippet>
<template>
<dom-bind>
<template is="dom-bind">
<geo-location latitude="{{latitude}}" longitude="{{longitude}}"></geo-location>
<geo-location permission-status="{{permissionStatus}}" latitude="{{latitude}}" longitude="{{longitude}}"></geo-location>

<ul>
<li>Permission status: [[permissionStatus]]</li>
<li>Latitude: [[latitude]]</li>
<li>Longitude: [[longitude]]</li>
</ul>
Expand Down
36 changes: 32 additions & 4 deletions geo-location.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,16 @@
<!--
Geolocation API Polymer web component.

Example to get the device geolocation values:
Get once and display the device geolocation values and continuous update the Geolocation API permission status:

```html
<geo-location latitude="{{latitude}}" longitude="{{longitude}}"></geo-location>
<geo-location permission-status="{{permissionStatus}}" latitude="{{latitude}}" longitude="{{longitude}}"></geo-location>

<ul>
<li>Permission status: [[permissionStatus]]</li>
<li>Latitude: [[latitude]]</li>
<li>Longitude: [[longitude]]</li>
</ul>
```

Continuous update the device geolocation values with high accuracy, and center Google Maps map and marker to the current location:
Expand Down Expand Up @@ -85,15 +91,15 @@
},

/**
* The maximumAge option in the Gelocation API.
* The maximumAge option in the Geolocation API.
*/
maximumAge: {
type: Number,
value: 0
},

/**
* The timeout option in the Gelocation API.
* The timeout option in the Geolocation API.
*/
timeout: {
type: Number,
Expand All @@ -108,6 +114,15 @@
notify: true,
readOnly: true,
value: null
},

/**
* [Status](https://w3c.github.io/permissions/#status-of-a-permission) of Geolocation API permission.
*/
permissionStatus: {
type: String,
readOnly: true,
notify: true
Copy link
Owner

Choose a reason for hiding this comment

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

needs a default state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ebidel Is not the default value optional? Should I use value: null?

Copy link
Owner

Choose a reason for hiding this comment

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

null sounds good. As long as that's not one of the possible values the API returns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean that if we do not specify a default value at all, it is equivalent to null. Should I specify it explicitly?

}

},
Expand All @@ -134,6 +149,19 @@
* @param {Number} detail.longitude Longitude of the current position.
*/

attached: function() {
if ('permissions' in navigator) {
navigator.permissions.query({
name: 'geolocation'
}).then(function(permissionStatus) {
this._setPermissionStatus(permissionStatus.state);
Copy link
Owner

Choose a reason for hiding this comment

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

need to handle the permissions api not being available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ebidel What should we do if Permissions API is not available?

Copy link
Owner

Choose a reason for hiding this comment

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

Probably fail silently, but the permissionStatus needs a default value or some way to signal to users that permission state is unknown.

permissionStatus.onchange = function() {
Copy link
Owner

Choose a reason for hiding this comment

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

What happens if this element gets attached/detached/attached from the DOM? Does permissionStatus get gc'd properly and we're not adding creating a bunch of orphaned change handlers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ebidel I don't know. Could you please help me do this in a better way?

Copy link
Owner

Choose a reason for hiding this comment

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

As the author of this PR, I was hoping you could do that research.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ebidel BTW, what about placing this code inside ready lifecycle callback instead of attached? Is it a bad idea?
attached can be called multiple times during the lifetime of an element. The first attached callback is guaranteed not to fire until after ready.

this._setPermissionStatus(permissionStatus.state);
}.bind(this);
}.bind(this));
}
},

detached: function () {
this._clearWatch(this._watch);
},
Expand Down