-
Notifications
You must be signed in to change notification settings - Fork 72
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
[FE-8035] Upgrade Mapbox #373
Conversation
mapbox isn't meant to be in a node environment, and therefore fails when we run unit tests
We're going to need to produce un-premultiplied images from the BE, otherwise immerse will be limited to opaque colors only, and there can certainly be scenarios where that is unwanted. Here's the BE ticket: https://jira.omnisci.com/browse/BE-3956 |
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.
lgtm; couldn't break anything
@@ -2,7 +2,6 @@ import d3 from "d3" | |||
import * as _ from "lodash" | |||
import { redrawAllAsync, resetRedrawStack } from "../core/core-async" | |||
import { utils } from "../utils/utils" | |||
import { mapDrawMixin } from "./map-draw-mixin" |
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.
Thanks for removing this.
If light-v9 is the only option that is comparable to light-v8, we can go with it. Seems like all functionalities are working fine based on my testing. Slight opacity difference in general but assuming we are ok with that. Only thing needed to get fixed is the slider value should reflect the current opacity for the multilayers. |
One more thing @CaitW, is your immerse build is up to date? Does it include the raster resize fix that REMOVES |
@@ -760,6 +760,9 @@ function genLayeredVega(chart) { | |||
const vegaSpec = { | |||
width: Math.round(width), | |||
height: Math.round(height), | |||
viewRenderOptions: { |
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 the trick BE needs?
Looks like there is outstanding PR in mapbox.gl mapbox/mapbox-gl-js#8693 to add originalEvent for the NavigationControl buttons. We can probably use that to call UpdateDraw on the +/- zoom button to immediately update the shapes. |
Immerse PR
Mapbox Upgrade
Initial investigation notes + reasoning for the upgrade
TLDR: we forked Mapbox a long time ago and never updated it. We'd like to use the current version of Mapbox and not rely on a fork.
This affects all map charts and scatterplot.
Major Changes
Adjusting for premultiplied alpha
Rendering returns premultiplied images.
As an intermediate solution, I changed things so that we request a fully-opaque image from core and then apply the transparency client-side. This seems to create a chart that most closely resembles the given color scale.I'm don't know enough about this subject to know whether this is the best solution. @vastcharade suggested potentially requesting non-premultiplied images from core. See relevant BE IssueBackend pushed a fix for this, so all I did was revert the changes I noted in
strikethroughabove and add this to thegenLayedVega
call:Additional changes:
mapDrawMixin
because it doesn't appear to be usedmapbox-ported-functions.js
Arbitrary decisions up for debate:
light-v8
tolight-v9
(alters the basemap)axios
for getting an image