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

Shared color axes #3803

Merged
merged 14 commits into from
Apr 29, 2019
Merged

Shared color axes #3803

merged 14 commits into from
Apr 29, 2019

Conversation

etpinard
Copy link
Contributor

resolves #3431 - to be merged in #3786

@plotly/plotly_js please review this PR commit-by-commit. The "main" commits are:

  • ae3f407 where layout.coloraxis? attribute declarations and default logic are added.
  • 5bb341e implements the cross-trace-defaults, calc and colorbar drawing.
  • bd81ac2 handles the contour trace edge cases.

- 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.
@etpinard etpinard added this to the v1.48.0 milestone Apr 24, 2019
@@ -90,7 +91,7 @@ function makeColorBarData(gd) {

if(cont && cont.showscale) {
var opts = cont.colorbar;
opts._id = 'cb' + trace.uid;
opts._id = 'cb' + trace.uid + (allowsMultiplotCbs && contName ? '-' + contName : '');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This commits essentially resolves #3555

I didn't add support for marker.line. colorbars (yet) - let me know you think that's a good idea.

} else {
// re-coerce colorscale attributes w/o coloraxis
for(var i = 0; i < stash[2].length; i++) {
stash[2][i]();
Copy link
Contributor Author

Choose a reason for hiding this comment

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


var cAttrs = ['cauto', 'colorscale', 'reversescale'];
cAttrs.forEach(function(attr) {
expect(fullLayout.coloraxis[attr]).not.toBe(undefined, 'coloraxis ' + attr);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please note that layout.coloraxis? use cmin, cmax and cauto no matter which trace types make reference to them. In other words, setting zmin or zmax in a coloraxis won't do anything.

if(min === undefined) {
min = minVal();
} else if(auto) {
if(container._colorAx && isNumeric(min)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how we get all traces sharing the same coloraxis to contribute to the auto cmin / cmax computations.

@archmoj
Copy link
Contributor

archmoj commented Apr 26, 2019

Wondering could we apply this on bars with line and marker colorscales?
See codepen.

@etpinard
Copy link
Contributor Author

etpinard commented Apr 26, 2019

Wondering could we apply this on bars with line and marker colorscales?

We could. But as mentioned in #3803 (comment) - I'm not 100% sure adding colorbar to marker.line colorscales is a necessary and/or a good idea.

Edit: I misread your comment. Here's an example: https://codepen.io/etpinard/pen/bJOLam?editors=0010

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!
@etpinard please find my questions below.
Also it would be nice to add a new gl3d mock or edit gl3d_ribbons.
Thanks.

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

Also it would be nice to add a new gl3d mock or edit gl3d_ribbons.

Thanks for the tip @archmoj . It turns out gl3d needed a bit more work than anticipated. See 1d4d06f

@archmoj
Copy link
Contributor

archmoj commented Apr 29, 2019

Awesome PR.
Good to merge into #3786.
💃

@etpinard etpinard merged commit 4107ce3 into colorbar-refactor Apr 29, 2019
@etpinard etpinard deleted the coloraxes-finalist branch April 29, 2019 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants