Improve `mapbox.style` description + use mapbox-gl v1.1.1 by etpinard · Pull Request #4035 · plotly/plotly.js

etpinard · 2019-07-11T15:23:56Z

@plotly/plotly_js @nicolaskruchten

antoinerg · 2019-07-11T15:31:15Z

This description is much clearer 👍

antoinerg · 2019-07-11T15:32:25Z

💃 from me

etpinard · 2019-07-11T17:59:43Z

More from @nicolaskruchten on slack:

what does this line mean? https://github.com/plotly/plotly.js/blob/master/src/plots/mapbox/layout_attributes.js#L141

Is this line up to date wrt to raster and image source types? https://github.com/plotly/plotly.js/blame/master/src/plots/mapbox/layout_attributes.js#L120

would be good to clarify this line if it only applies when sourcetype is geojson: https://github.com/plotly/plotly.js/blob/master/src/plots/mapbox/layout_attributes.js#L143

these will be addressed in this PR.

nicolaskruchten · 2019-07-11T18:00:36Z

Would be good to point out that this is only needed for certain values of layout.mapbox.style: https://github.com/plotly/plotly.js/blob/master/src/plots/mapbox/layout_attributes.js#L40

nicolaskruchten · 2019-07-11T18:02:01Z

The relationship between layers.type and layers.sourcetype is pretty nebulous to me at the moment... Would be good to clarify.

nicolaskruchten · 2019-07-12T13:05:02Z

The attr for choroplethmapbox.below here https://github.com/plotly/plotly.js/blob/master/src/traces/choroplethmapbox/attributes.js#L63 should be expanded a bit to say something like "the default value is 'water', which is the ID of a layer that is often present in Mapbox styles. If no layer with this ID is implicitly or explicitly defined via layout.mapbox.style or explicitly present in layout.mapbox.layers then this trace will be positioned as usual."

... or something. that's basically a clearer description of the current behaviour and assumptions IMO.

etpinard · 2019-07-12T15:15:02Z

The attr for choroplethmapbox.below here https://github.com/plotly/plotly.js/blob/master/src/traces/choroplethmapbox/attributes.js#L63 should be expanded a bit to say something like "the default value is 'water', which is the ID of a layer that is often present in Mapbox styles.

That's not exactly true, here's the logic:

plotly.js/src/traces/choroplethmapbox/plot.js

Lines 106 to 132 in 14133af

    
           proto.getBelow = function(optsAll) { 
        
               if(optsAll.below !== undefined) { 
        
                   return optsAll.below; 
        
               } 
        
               var mapLayers = this.subplot.map.getStyle().layers; 
        
               var out = ''; 
        
               // find layer just above top-most "water" layer 
        
               for(var i = 0; i < mapLayers.length; i++) { 
        
                   var layerId = mapLayers[i].id; 
        
                   if(typeof layerId === 'string') { 
        
                       var isWaterLayer = layerId.indexOf('water') === 0; 
        
                       if(out && !isWaterLayer) { 
        
                           out = layerId; 
        
                           break; 
        
                       } 
        
                       if(isWaterLayer) { 
        
                           out = layerId; 
        
                       } 
        
                   } 
        
               } 
        
               return out; 
        
           };

The default below value is the layer just above the last water* layer. It's unfortunate that mapbox-gl doesn't have an above attribute, it would have been easier to describe.

etpinard · 2019-07-18T20:12:01Z

Based off this branch's latest commit

https://github.com/plotly/plotly.js/blob/ca762e96d9f8ea77966cfe4918c292e48661f3b0/src/plots/mapbox/layout_attributes.js

here are some TODOs that will done on this branch:

Improve "mapbox token error message" mapbox token error message #4059 - done in a460be0
rewrite mapbox.style description as:

Defines the map layers that are rendered by default below the trace layers defined in `data`, 
which are themselves by default rendered below the layers defined in `layout.mapbox.layers`.

These layers can be defined either explicitly as a Mapbox Style object which can contain multiple 
layer definitions that load data from any public or private Web Map Service (WMS), or implicitly 
by using one of the builtin plotly.js style objects which use WMSes which do not require any 
access tokens, or by using a  default Mapbox style or custom Mapbox style URL, both of 
which require a Mapbox access token.

Mapbox Style objects are of the form described in the Mapbox GL JS documentation available at
https://docs.mapbox.com/mapbox-gl-js/style-spec

The built-in plotly.js styles objects are... (provide name and the tile source)

The built-in Mapbox styles are...

Mapbox style URLs are of the form...

done in b933528

mention new non-mapbox-server style shortcuts (once @antoinerg 's work is merged)

done in b933528

Clarify relationship between layers.type and layers.sourcetype - done in d81f28c
anything else?

nicolaskruchten · 2019-07-19T12:32:14Z

This is already a big set of improvements, thank you very much :)

One small adjustment to my initial proposal: the phrase public or private Web Map Service (WMS) should be expanded to say public or private Tile Map Service (TMS or XYZ) or Web Map Service (WMS)

https://github.com/mapbox/mapbox-gl-js/blob/master/CHANGELOG.md#111

... when trying to plot a mapbox subplot w/o a Mapbox access token and w/o setting a Mapbox-served style as opposed to simply throwing a "missing token" msg. Also, rename constants.styles -> constants.stylesNonMapbox

etpinard · 2019-07-22T20:04:07Z

@plotly/plotly_js @nicolaskruchten this PR is now reviewable again.

Note that I merged master (with @antoinerg 's attributions and non-mapbox styles work) and I bumped mapbox-gl to v1.1.1 - so this is essentially on-track to be the "last" mapbox PR for 1.49.0 😏

nicolaskruchten · 2019-07-23T13:48:57Z

                'Source can be either a URL,',
                'a geojson object (with `sourcetype` set to *geojson*)',
-                'or an array of tile URLS (with `sourcetype` set to *vector*).'
+                'or an array of URLs (with `sourcetype` set to *vector* or *raster*).'


what does sourcetype have to be if source is a URL?

And actually I would leave the word "tile" in there, as it's not super clear what the URL should point to otherwise. We should try to say what the non-array URL should point to also: an image? a GeoJSON file?

Now this is:

plotly.js/src/plots/mapbox/layout_attributes.js

Lines 137 to 142 in 29c2bf9

'Sets the source data for this layer (mapbox.layer.source).',

'When `sourcetype` is set to *geojson*, `source` can be a URL to a GeoJSON',

'or a GeoJSON object.',

'When `sourcetype` is set to *vector* or *raster*, `source` can be a URL or',

'an array of tile URLs.',

'When `sourcetype` is set to *image*, `source` can be a URL to an image.'

nicolaskruchten · 2019-07-23T13:50:10Z

+                'With `sourcetype` set to *geojson*, *circle*, *line*, *fill* and *symbol* are available',
+                'but note that *line* and *fill* are not compatible with Point',
+                'GeoJSON geometries.',
+                'With `sourcetype` set to *vector*, *circle*, *line*, *fill* and *symbol* are available.',


Suggested change

'With `sourcetype` set to *vector*, *circle*, *line*, *fill* and *symbol* are available.',

'With `sourcetype` set to *vector*, the following values are allowed: *circle*, *line*, *fill* and *symbol*.',

Now this is:

plotly.js/src/plots/mapbox/layout_attributes.js

Lines 170 to 178 in 29c2bf9

'Sets the layer type,',

'that is the how the layer data set in `source` will be rendered',

'With `sourcetype` set to *geojson*, the following values are allowed:',

'*circle*, *line*, *fill* and *symbol*.',

'but note that *line* and *fill* are not compatible with Point',

'GeoJSON geometries.',

'With `sourcetype` set to *vector*, the following values are allowed:',

' *circle*, *line*, *fill* and *symbol*.',

'With `sourcetype` set to *raster* or `*image*`, only the *raster* value is allowed.'

nicolaskruchten · 2019-07-23T17:54:48Z

💃 thanks very much, this is a huge step up in terms of users' ability to bootstrap their understanding of how to do things :)

improve mapbox.style description

14ace10

etpinard added the type: documentation label Jul 11, 2019

chriddyp reviewed Jul 11, 2019

View reviewed changes

Comment thread src/plots/mapbox/layout_attributes.js Outdated

chriddyp reviewed Jul 11, 2019

View reviewed changes

Comment thread src/plots/mapbox/layout_attributes.js Outdated

chriddyp reviewed Jul 11, 2019

View reviewed changes

Comment thread src/plots/mapbox/layout_attributes.js Outdated

nicolaskruchten reviewed Jul 11, 2019

View reviewed changes

Comment thread src/plots/mapbox/layout_attributes.js Outdated

nicolaskruchten reviewed Jul 11, 2019

View reviewed changes

Comment thread src/plots/mapbox/layout_attributes.js Outdated

etpinard added this to the v1.49.0 milestone Jul 11, 2019

improve mapbox.* descriptions (take 2)

ca762e9

nicolaskruchten mentioned this pull request Jul 18, 2019

mapbox token error message #4059

Closed

etpinard added 6 commits July 22, 2019 14:14

Merge branch 'master' into mapbox-style-better-docs

1b15616

bump mapbox-gl to 1.1.1

d4a2308

https://github.com/mapbox/mapbox-gl-js/blob/master/CHANGELOG.md#111

resolves #4059 - throw error msg about "missing style"

a460be0

... when trying to plot a mapbox subplot w/o a Mapbox access token and w/o setting a Mapbox-served style as opposed to simply throwing a "missing token" msg. Also, rename constants.styles -> constants.stylesNonMapbox

use styleValuesNonMapbox in mapbox.style attr declaration

81f9d53

mprove mapbox.style description (take 3)

b933528

improve mapbox.layers[i] sourcetype and type attr descriptions

d81f28c

etpinard changed the title ~~Improve mapbox.style description~~ Improve mapbox.style description + use mapbox-gl v1.1.1 Jul 22, 2019

etpinard added status: reviewable bug something broken labels Jul 22, 2019

nicolaskruchten reviewed Jul 23, 2019

View reviewed changes

Comment thread src/plots/mapbox/layout_attributes.js

nicolaskruchten reviewed Jul 23, 2019

View reviewed changes

Comment thread src/plots/mapbox/layout_attributes.js

nicolaskruchten reviewed Jul 23, 2019

View reviewed changes

Comment thread src/plots/mapbox/layout_attributes.js Outdated

nicolaskruchten reviewed Jul 23, 2019

View reviewed changes

Comment thread src/plots/mapbox/layout_attributes.js Outdated

nicolaskruchten reviewed Jul 23, 2019

View reviewed changes

Comment thread test/jasmine/tests/mapbox_test.js

improve mapbox layer source and type descriptions (again)

29c2bf9

etpinard merged commit a9e7480 into master Jul 23, 2019

etpinard deleted the mapbox-style-better-docs branch July 23, 2019 18:09

	'Sets the source data for this layer (mapbox.layer.source).',
	'When `sourcetype` is set to geojson, `source` can be a URL to a GeoJSON',
	'or a GeoJSON object.',
	'When `sourcetype` is set to vector or raster, `source` can be a URL or',
	'an array of tile URLs.',
	'When `sourcetype` is set to image, `source` can be a URL to an image.'

	'With `sourcetype` set to vector, circle, line, fill and symbol are available.',
	'With `sourcetype` set to vector, the following values are allowed: circle, line, fill and symbol.',

	'Sets the layer type,',
	'that is the how the layer data set in `source` will be rendered',
	'With `sourcetype` set to geojson, the following values are allowed:',
	'circle, line, fill and symbol.',
	'but note that line and fill are not compatible with Point',
	'GeoJSON geometries.',
	'With `sourcetype` set to vector, the following values are allowed:',
	' circle, line, fill and symbol.',
	'With `sourcetype` set to raster or `image`, only the raster value is allowed.'

Uh oh!

Uh oh!

Conversation

etpinard commented Jul 11, 2019

Uh oh!

antoinerg commented Jul 11, 2019

Uh oh!

antoinerg commented Jul 11, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

etpinard commented Jul 11, 2019

Uh oh!

nicolaskruchten commented Jul 11, 2019

Uh oh!

nicolaskruchten commented Jul 11, 2019

Uh oh!

nicolaskruchten commented Jul 12, 2019

Uh oh!

etpinard commented Jul 12, 2019

Uh oh!

etpinard commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolaskruchten commented Jul 19, 2019

Uh oh!

etpinard commented Jul 22, 2019

Uh oh!

Uh oh!

Uh oh!

nicolaskruchten Jul 23, 2019

Choose a reason for hiding this comment

Uh oh!

nicolaskruchten Jul 23, 2019

Choose a reason for hiding this comment

Uh oh!

etpinard Jul 23, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicolaskruchten Jul 23, 2019

Choose a reason for hiding this comment

Uh oh!

etpinard Jul 23, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nicolaskruchten commented Jul 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

etpinard commented Jul 18, 2019 •

edited

Loading