-
Notifications
You must be signed in to change notification settings - Fork 75
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
perf: Add basemaps from Carto #1107
Conversation
Since we switch to semantic-release, can you prefix the name of the PR with One of our examples shows off the list of defined tilesets (see examples/measure/index.pug). Can you add these to it? Perhaps as a separate PR, we could add preferred human-readable labels to these definitions and then populate the example from the dictionary of values. |
src/osmLayer.js
Outdated
@@ -139,6 +142,18 @@ let StamenAttribution = 'Map tiles by <a href="http://stamen.com">Stamen ' + | |||
* @type {object} | |||
*/ | |||
osmLayer.tileSources = { | |||
'darkMatter-with-labels': { |
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.
For consistency, should this be dark-matter-with-labels
rather than camel case?
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.
Yes, yes it should.
I can certainly add these to the example and make those changes. |
I added name to the tileSources definition and changed the example to load the tileSources dynamically. Let me know what you think. |
Great. |
src/osmLayer.js
Outdated
'dark-matter-with-labels': { | ||
url: ' https://{s}.basemaps.cartocdn.com/rastertiles/dark_all/{z}/{x}/{y}.png', | ||
attribution: CartoAttribution, | ||
name: 'Dark Matter With Labels', |
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.
My only recommendation is to prefix each of the Carto names with "Carto ".
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 all set now
🎉 This PR is included in version 1.4.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Adds base maps from:
https://carto.com/help/building-maps/basemap-list/#carto-vector-basemaps
Attribution modeled after documentation at:
https://carto.com/help/working-with-data/attribution/#basemaps