-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Mapbox raster #3928
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
Mapbox raster #3928
Conversation
8da7765
to
d57e8a3
Compare
Thanks for the PR @hishamkaram ! We're in the process of adding a few mapbox-related features and bumping the mapbox-gl version (they made it to v1 🎉 ), so I think it might be best to wait a few week before thinking about merging your work. In the meantime, could you provide a "data"/"layout" JSON mock that showcases a raster layer on a mapbox subplot? Sorry for the inconvenience and thanks again for your contribution. |
Ping @hishamkaram - are you still interested in bringing this PR to the finish line? To do so, we'll need add a 'data/layout' mock (example) |
sourceOpts.tileSize = 256; | ||
for(var index = 0; index < source.length; index++) { | ||
var url = source[index]; | ||
source[index] = decodeURIComponent(url); |
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 necessary? I would expect the URLs in the mapbox.layers[i]
setting to already be decoded.
@@ -132,7 +132,7 @@ var attrs = module.exports = overrideAll({ | |||
|
|||
type: { | |||
valType: 'enumerated', | |||
values: ['circle', 'line', 'fill', 'symbol'], | |||
values: ['circle', 'line', 'fill', 'symbol', 'raster'], |
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.
As type: 'raster'
is the only value that "makes sense" for sourcetype: 'raster'
, we should probably override (wrong) user settings during the defaults step:
plotly.js/src/plots/mapbox/layout_defaults.js
Lines 53 to 90 in c0cebfd
if(visible) { | |
var sourceType = coerce('sourcetype'); | |
coerce('source'); | |
if(sourceType === 'vector') coerce('sourcelayer'); | |
// maybe add smart default based off GeoJSON geometry? | |
var type = coerce('type'); | |
coerce('below'); | |
coerce('color'); | |
coerce('opacity'); | |
coerce('minzoom'); | |
coerce('maxzoom'); | |
if(type === 'circle') { | |
coerce('circle.radius'); | |
} | |
if(type === 'line') { | |
coerce('line.width'); | |
coerce('line.dash'); | |
} | |
if(type === 'fill') { | |
coerce('fill.outlinecolor'); | |
} | |
if(type === 'symbol') { | |
coerce('symbol.icon'); | |
coerce('symbol.iconsize'); | |
coerce('symbol.text'); | |
Lib.coerceFont(coerce, 'symbol.textfont'); | |
coerce('symbol.textposition'); | |
coerce('symbol.placement'); | |
} | |
} |
Down the road, it might be nice to expose some of mapbox-gl's raster style settings:
https://docs.mapbox.com/mapbox-gl-js/style-spec/#layers-raster
@@ -131,7 +131,7 @@ function isVisible(opts) { | |||
|
|||
return opts.visible && ( | |||
Lib.isPlainObject(source) || | |||
(typeof source === 'string' && source.length > 0) | |||
((typeof source === 'string' || Array.isArray(source)) && source.length > 0) |
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.
Nice, this means both:
Plotly.newPlot('graph', [{type: 'scattermapbox'}], {
mapbox: {
layers: [{
sourcetype: 'raster',
source: 'mapbox://mapbox.satellite',
type: 'raster'
}]
}
})
and
Plotly.newPlot('graph', [{type: 'scattermapbox'}], {
mapbox: {
layers: [{
sourcetype: 'raster',
source: [
'https://a.tile.openstreetmap.org/{z}/{x}/{y}.png',
'https://b.tile.openstreetmap.org/{z}/{x}/{y}.png'
],
type: 'raster'
}]
}
})
will work
@@ -100,7 +100,7 @@ var attrs = module.exports = overrideAll({ | |||
}, | |||
sourcetype: { | |||
valType: 'enumerated', | |||
values: ['geojson', 'vector'], | |||
values: ['geojson', 'vector', 'raster'], |
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.
At some point, we could add 'image'
- https://docs.mapbox.com/mapbox-gl-js/style-spec/#sources-image
- https://stackoverflow.com/questions/50874220/how-do-i-add-a-simple-image-overlay-in-mapbox-javascript
which could be used together with mapbox.layers[i].type: 'raster'
Closing in favour of #4006 |
this PR include: