Introducing plotly.js + Mapbox GL #626
Conversation
- N.B. 'docalc' is more conservative then 'doplot'
- that setups build files containing mapbox crendentials so that test runners can load them seamlessly.
- note that a MAPBOX_ACCESS_TOKEN env variable has been set on the CircleCI plotly.js project.
- this commit will be remove before the first release as some mapbox cannot be tested on CircleCI yet.
- as FF launched by karma can't create a webgl context on CircleCI.
- as mapbox toImage blows the image generation queue. - note that generating image one-by-one on CircleCI works fine but very slowly.
|
🎉 Will start reviewing this asap! |
|
In plotly.js terms, |
| @@ -87,6 +87,9 @@ module.exports = { | |||
| // URL to topojson files used in geo charts | |||
| topojsonURL: 'https://cdn.plot.ly/', | |||
|
|
|||
| // Mapbox access token (required to plot mapbox trace types) | |||
| mapboxAccessToken: null, | |||
There was a problem hiding this comment.
N.B. an important distinction for mapbox subplots.
|
|
||
| // remove empty trace objects | ||
| var ids = Object.keys(traceHash); | ||
| id_loop: |
There was a problem hiding this comment.
🐄 I've never been a big fan of this - doesn't seem to be necessary in this instance either.
Edit: Nvm I see why. Still not a fan of it though 😑
| description: [ | ||
| 'Sets the pitch angle of the map', | ||
| '(in degrees, where *0* means perpendicular to the surface of the map).' | ||
| ].join(' ') |
- also tweak factor for cleaner look - add bubble with text image test
| @@ -93,7 +94,7 @@ module.exports = function convert(calcTrace) { | |||
| 'text-field': '{text}' | |||
| }); | |||
|
|
|||
| if(hasMarkers) { | |||
| if(hasSymbols) { | |||
There was a problem hiding this comment.
@chriddyp this should fix the console errors we were seeing on Friday.
|
Great tests! I'm gonna say 💃 on this one - there's small things we can improve on and fixup along the way, but let's get it in quick for @jackparmer! ✨ clean! |
- so that links to open-street map don't show.
This reverts commit a47704c.
as `scattermapbox' isn't part of lib/index.js module
|
💥 🚢 ! |
Have you ever asked yourself: wouldn't it be great to plot Mapbox GL maps with the plotly.js API?
Well, this PR will make your day! 🎉
In more details, this PR adds one new plot type (fittingly named
mapbox) and one new trace typescattermapboxto plotly.js.IMPORTANT: This PR won't make
scattermapboxpart of our official plotly.jsdistbundle. Thescattermapboxtrace type will only be available through custom bundling, similar toheatmapglandcontourgl. Why? See Testing issues below.Features
markers,linesandtextscatter modestoselffills.connectgapstrue or falsemaker.colorandmarker.sizewhen circlemarker.symbol.marker.symbolrestyle,relayoutandextendTracessupportLimitations
line.dashsupport. I ran into some issues onrestyle. I think it is an issue withmapbox-gl. Further investigation is needed.marker.linestyling options.mapbox-gldoesn't natively support them. I think it's worth waiting for them instead of hacking something in.locationsas an alternatively tolon/latcoordinates like inscattergeo.arrayOksupport, as mentioned in the features section.Testing issues
I ran into some issues while attempting to run our image tests with the new mapbox mocks on CircleCI. Moreover, our jasmine test runner currently can't create WebGL context on CircleCI (see #241 for more info). So, this leaves this our mapbox integration code untested on CircleCI for now. The tests are working fine on my laptop.