Skip to content
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

Bump mapbox gl to 0.44.0 #2361

Merged
merged 15 commits into from Feb 14, 2018

Conversation

Projects
None yet
3 participants
@etpinard
Copy link
Member

etpinard commented Feb 12, 2018

resolves #1714 and makes #2029 obsolete.

It's about time we do this. Note that mapbox-gl@0.44.0 is still incompatible with nw@0.12 that we use in our imagetest container (ref #2029 (comment)), meaning we'll have to manually test mapbox subplot image generation. But yeah after some discussing with @jackparmer, this is way better than sticking to a almost two year old version of mapbox-gl.

A lot of things have changed from mapbox-gl version 0.22.0 to 0.44.0. Our tests did catch many of those changes. But still, we'll need to manually test this thing thoroughly before merging.

This PR also adds support for (un)selected marker size and color on top of marker opacity for scattermapbox traces #2346, fixes a hover bug and improves first-render performance.

Some TODOs:

  • thorough manual testing
  • write more convert test for selected / unselected attributes - done in 1998f90

etpinard added some commits Feb 6, 2018

add 'some' mapbox-gl css rule
... mostly to hide mapbox-gl's own no-css warning
    that get injected into the DOM.
resolves #2346 - add (un)selected.marker color/size support
... to scattermapbox traces, along with fixing existing bugs
    and cleaning up the convert logic (in 0.44.0, we no longer
    have to generate a hacky hash stops array 🎉
make scattermapbox first render faster
- by setting up paint and layout props during the 1st addLayer call
  and by setting up the source during the 1st addSource call,
  as opposed to having an blank 'init' and a generic 'update' step.
@jackparmer

This comment has been minimized.

Copy link
Contributor

jackparmer commented Feb 12, 2018

Happy to manually test this tonight or tomorrow on stage!

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

alexcjohnson commented Feb 12, 2018

meaning we'll have to manually test mapbox subplot image generation.

What do you mean by this? Just that we have tests that can't run on CI but can run locally, or are there things we have to run in a browser and eyeball?

@etpinard

This comment has been minimized.

Copy link
Member Author

etpinard commented Feb 13, 2018

What do you mean by this? Just that we have tests that can't run on CI but can run locally, or are there things we have to run in a browser and eyeball?

I added a command in tasks/noci_test.sh that re-generates the mapbox baselines using image-exporter. If this doesn't generate a git diff we're good. We could use something more evolved at some point, depending how long we'll have to wait before merging #1972.

@etpinard

This comment has been minimized.

Copy link
Member Author

etpinard commented Feb 13, 2018

After some testing help from @jackparmer, tagging this thing as reviewable.

expect(evt.clientX).toEqual(pointPos[0], 'event.clientX');
expect(evt.clientY).toEqual(pointPos[1], 'event.clientY');
expect(evt.clientX).toBeDefined('event.clientX');
expect(evt.clientY).toBeDefined('event.clientY');

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Feb 13, 2018

Contributor

why did these have to change?

This comment has been minimized.

Copy link
@etpinard

etpinard Feb 13, 2018

Author Member

Oops. My mistake. I shouldn't have committed this. Good 👁 Reverted in 0ef2ba6

@@ -46,6 +46,29 @@
"opacity": 0.5

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Feb 13, 2018

Contributor

Looks (from the baseline image change) like this didn't actually work before, but now it does 🎉
Was that a known bug?

This comment has been minimized.

Copy link
@etpinard

etpinard Feb 13, 2018

Author Member

Yes (after some initial confusion) -> #2346


// shift longitude to [-180, 180] to determine closest point
var lonShift = winding * 360;
var xval2 = xval - lonShift;

function wrapLon(lon) {
return Lib.wrap180(lon);
}

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Feb 13, 2018

Contributor

this wrapper seems unnecessary

@@ -439,8 +439,8 @@ drawing.pointStyle = function(s, trace, gd) {
});
};

drawing.selectedPointStyle = function(s, trace) {
if(!s.size() || !trace.selectedpoints) return;
drawing.makeSelectedPointStyleFns = function(trace) {

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Feb 14, 2018

Contributor

Nice abstraction here 👌

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

alexcjohnson commented Feb 14, 2018

@etpinard this looks great! We'll need to sort out fonts before we can use image_exporter for everything (just shows up in the legend font in the mapbox_0 and mapbox_connectgaps images, it's a totally different font, look particularly at the c's and g's), but it's OK for this PR. It might mean that you're the only one who can run that noci_test script successfully, but if you're OK with that possibility then so am I 😉

💃 after the one last minor comment and tests pass.

@etpinard

This comment has been minimized.

Copy link
Member Author

etpinard commented Feb 14, 2018

We'll need to sort out fonts before we can use image_exporter for everything (just shows up in the legend font in the mapbox_0 and mapbox_connectgaps images

Ok. Referencing #1972

@etpinard etpinard merged commit 4aa511a into master Feb 14, 2018

5 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test-image Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine2 Your tests passed on CircleCI!
Details
ci/circleci: test-syntax Your tests passed on CircleCI!
Details

@etpinard etpinard deleted the bump-mapbox-gl branch Feb 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.