Colorbar refactor #3786
Merged
Colorbar refactor #3786
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- the colorbar module used to use a d3-like closure pattern which no other plotly.js module use - colorbar logic was invoke per-trace at various moments in the plot pipeline, making hard to work with
- colorbar.draw is now invoked alongside the other 'margin-pusher' components (e.g. legend, updatemenus ...) - colorbar.draw draws ALL colorbar on a graph, by looping over the data to find how many colorbars there are - use d3-update pattern to remove old colorbars - colorbar.draw handles updates, simplifying doColorbars subroutine - contour.colorbar defines a 'calc' method to generate its special colorbar options
- not sure what's going on here, the generated SVG layers are the same before/after refactor. - I suspect some automargin funniness is causing this, but I haven't found evidence that the updated baselines are wrong.
etpinard
commented
Apr 17, 2019
| if(d[0] && d[0].t && d[0].t.cb) d[0].t.cb(); | ||
| }); | ||
|
|
||
| Registry.getComponentMethod('colorbar', 'draw')(gd); |
There was a problem hiding this comment.
This commit essentially reverts 84c9708
No need to update the baselines!
|
Looks very good to me. |
|
This PR 🔪 116 lines of code without changing any baselines. Great job @etpinard
Sounds like a good idea indeed. 💃 |
|
THanks for the reviews guys. I'm holding off merging this thing. I'll thinking of potentially releasing a 1.47.4 tomorrow or Thursday before merging things for 1.48.0 |
- adapt multiple trace attributes.js files accordingly - standard colorscale/attributes import to colorScaleAttrs
- 'line' is now declared within _module.colorbar, so we don't need this block - add scatter3d test for good measure
- reuse Colorscale.supplyDefaults - use 'cmin', 'cmax' and 'cauto' no matter the trace types plotted - keep track of color axes referenced in the traces in fullLayout._colorAxes - ignore coloraxis referenced when colorbar visuals are incompatible - tweak PlotSchema.get() for layout.colorscale / layout.coloraxis? edge cases
... to help with per-trace min/max/auto or per-coloraxis defs
... and add makeColorScaleFuncFromTrace wrapper to DRY things up
a little bit.
- use min of min (and max of max) of all traces linked to same color axis to compute min/max when auto:true - call Colorscale.crossTraceDefaults after having relinked layout, to use coloraxis._min, coloraxis._max -
- which has special logic which currently mutates zmin/zmax after the calc step.
- remove zmin/zmax mutations happening after the calc stpe
- make traceIs('contour') traces call Colorscale.calc with
their own options.
Merged
|
OK to merge @plotly/plotly_js ? |
|
Nicely done. 💃 💃 |
etpinard
added a commit
that referenced
this pull request
Sep 9, 2019
... post colorbar refactor of
#3786
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In brief,
colorbarlogic is now invoked in a very similar way to the other "margin-pusher" components (e.g. legend, updatemenus, etc.) - which should help us add colorbar features in the (near) feature.See the commit description of f7c5a8e for more details.
cc @plotly/plotly_js
Going forward, I'm thinking of perhaps merging
components/colorbar/intocomponents/colorscale/. I can't think of a good reason for keeping them distinct. Some reasons: (1)Colorscalerequires theColorbardefaults, (2) most (if not all) trace modules manually extend their colorscale attributes with thecolorbarattributes, (3)Colorscaledoesn't have adrawmethod. Let me know what you think.