-
Notifications
You must be signed in to change notification settings - Fork 92
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
Upgrade ambient-map to Polymer3.0+LitElement+MWC #81
Conversation
ambient-map/index.html
Outdated
<body class="fullbleed"> | ||
<script type="module" src="src/ambientmap-app.js"></script> | ||
</head> | ||
<body class="fullbleed"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this fullbleed used?
ambient-map/src/ambientmap-app.js
Outdated
this.latitude = 60.1699; | ||
this.longitude = 24.9384; | ||
this.mapApiKey = location.origin === 'https://ambientmap.appspot.com' || location.origin === 'https://intel.github.io' ? 'AIzaSyCR59cBBxaiINBUEbdmUVHYs8CV8jtEDTw' : 'DEADBEEF59cBBxaiINBUEbdmUVHYs8CV8jtEDTw'; | ||
this.infoText = "Welcome to Ambitent Map demo! This web application demonstrates how Ambient light sensor can be used to control style of a map widget. When ambient illuminance level is less than 10 lumen, night mode style will be used."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just inline these
ambient-map/src/ambientmap-app.js
Outdated
this.infoText = "Welcome to Ambitent Map demo! This web application demonstrates how Ambient light sensor can be used to control style of a map widget. When ambient illuminance level is less than 10 lumen, night mode style will be used."; | ||
this.styles = []; | ||
this.nightStyles = [ | ||
{elementType: 'geometry', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newline after {
ambient-map/src/ambientmap-app.js
Outdated
navigator.geolocation.getCurrentPosition((pos) => { | ||
this.latitude = pos.coords.latitude; | ||
this.longitude = pos.coords.longitude; | ||
this.requestUpdate('latitude'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better names these properties (static get properties()) when they update often. requestUpdate
is mostly needed when you deal with arrays etc
ambient-map/src/ambientmap-app.js
Outdated
} | ||
|
||
openInfoDialog() { | ||
this.infoDialog = this.shadowRoot.querySelector('#infoDialog'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done in firstUpdated (meaning filled shadow root exists)
} | ||
</style> | ||
|
||
<google-map id="mapElement" latitude="${this.latitude}" longitude="${this.longitude}" zoom="14" min-zoom="9" max-zoom="20" styles="${this.styles}" language="en" api-key="${this.mapApiKey}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These update often, so better use property setters than attribtute setters... this .latitute=
- Delete unused CSS class - Delete infoText attribtute - Add latitude and longitude properties - Get paper-dialog element in firstUpdated() - Add autofocus to close button - Modify JSON format
@kenchris, many thanks for your review! I just submit a new commit to address your comments, PTAL. |
@kenchris, @rakuco, PTAL.