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

Don't mutate colorscale, cmin/cmax and zmin/zmax into user traces #3341

Merged
merged 12 commits into from
Dec 17, 2018

Conversation

etpinard
Copy link
Contributor

This PR fixes two bugs (#3273 and #3100) using a similar technique to #3044.

In brief, we use Colorscale.crossTraceDefaults to put _ attributes (relinked in relinkPrivateKeys) into fullData - instead of mutating values into gd.data.

I also cleaned up a few things in components/colorscale/:

  • merge helpers into two files: scales.js and helpers.js
  • apply reversescale later in the pipeline (just before plotting) instead of mutating the colorscale value in gd._fullData (saving us some headaches in crossTraceDefaults).

asking @plotly/plotly_js for a review 😏

@nicolaskruchten would mind testing out 6a47d66 in the RCE? Thank you!

- that we don't mutate fullData[i].colorscale in defaults
  and or calc.
- to 'relink' zmin/zmax, cmin/cmax and auto colorscale,
  these values are computed in 'calc' in need to be relinked
  in 'supply-defaults' in order to work properly after edits
  that don't go through 'calc'.
... trace that have `autocolorscale:true` by default
... locking down the fix for #2873 completed in
    c9d92d7
@nicolaskruchten
Copy link
Member

Oh wow that works great :)

Copy link
Contributor

@archmoj archmoj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR! It may likely hit the record of the number of source files changed in one PR in 2018?
@etpinard After a quick first pass & concerning the change in this baseline; I was wondering which version is correct, old or new? The background color in the hover popup does not display the new color.

src/components/colorscale/calc.js Outdated Show resolved Hide resolved
@etpinard
Copy link
Contributor Author

After a quick first pass & concerning the change in this baseline; I was wondering which version is correct, old or new?

Thanks very much @archmoj ! It turns out both versions were wrong! The version from commit 0a686dd looked worse than the one on master, as now we no longer flip gd._fullData[i].colorscale when reversescale is true, but it was off nonetheless. See fix in 175626b as well as other corrected baselines in fc884a7

@nicolaskruchten
Copy link
Member

Out of curiosity, what's the new logic for the legend marker?

@etpinard
Copy link
Contributor Author

Out of curiosity, what's the new logic for the legend marker?

There's no new logic really. This PR fixed Lib.minExtend for colorscale values. Lib.minExtend is used in the legend-styling code to mock calcdata so that we can reuse our cartesian Drawing methods to draw legend items. Commit 0a686dd made this bug more obvious, but things were broken before.

@nicolaskruchten
Copy link
Member

OK, but what's the one-liner english description of the logic? We grab a marker from the middle of the scale or... ?

@etpinard
Copy link
Contributor Author

OK, but what's the one-liner english description of the logic?

We grab the first marker.color item

dEdit.mc = boundVal('marker.color', pickFirst);

and then apply the marker.colorscale if present.

@archmoj
Copy link
Contributor

archmoj commented Dec 17, 2018

Many thanks @etpinard
Big improvements indeed!
💃 from my side.

@etpinard
Copy link
Contributor Author

Thanks very much for the thorough review!

@etpinard etpinard merged commit e398676 into master Dec 17, 2018
@etpinard etpinard deleted the colorscale-no-mutate branch December 17, 2018 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants