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

use marker.colors with colorscale in treemap and sunburst plots #4242

Merged
merged 13 commits into from
Oct 7, 2019

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Oct 1, 2019

This PR adds ways to use marker.colors numbers with colorscale.
Also to mention the marker.colorscale would be defaulted if marker.colors is not empty.

Other changes within this PR also include:

  • implement marker.depthfade and drop marker.opacitybase and marker.opacitystep.
  • default pathbar.opacity to 1.

@etpinard
cc: @nicolaskruchten

- default marker.colorscale when having marker.colors
@archmoj archmoj added bug something broken status: reviewable labels Oct 1, 2019
@etpinard etpinard added this to the v1.50.0 milestone Oct 2, 2019
- also update the treemap baseline which now fails both locally and on the CI
 - drop opacitybase
 - drop opacitystep
 - combine colors with the background color according with their depth from the top
src/traces/treemap/style.js Outdated Show resolved Hide resolved
src/traces/treemap/plot.js Outdated Show resolved Hide resolved
@etpinard
Copy link
Contributor

etpinard commented Oct 4, 2019

Awesome job @archmoj !!

As for the failing tests:

it('should not coerce *marker.opacitybase* and *pathbar.opacity* when having *colorscale*', function() {
_supply([
{labels: [1], parents: ['']},
{labels: [1], parents: [''], marker: {colorscale: 'Blues'}}
]);
expect(fullData[0].marker.opacitybase).toBe(0.5);
expect(fullData[0].pathbar.opacity).toBe(0.5);
expect(fullData[1].marker.opacitybase).toBe(undefined, 'not coerced');
expect(fullData[1].pathbar.opacity).toBe(undefined, 'not coerced');
});

  • This one should not test the marker.depthfade isn't coerced when a marker colorscale is present.
  • We should also add one default test checking that marker.depthfade defaults to false when marker.colors is set

it('should be able to restyle *marker.opacitybase*', function(done) {
var mock = {
data: [{
type: 'treemap', pathbar: { visible: false },
labels: ['Root', 'A', 'B', 'b', 'b2', 'b3'],
parents: ['', 'Root', 'Root', 'B', 'b', 'b2']
}]
};
function _assert(msg, exp) {
return function() {
var layer = d3.select(gd).select('.treemaplayer');
var opacities = [];
layer.selectAll('path.surface').each(function() {
opacities.push(this.style.opacity);
});
expect(opacities).toEqual(exp, msg);
// editType:style
if(msg !== 'base') {
expect(Plots.doCalcdata).toHaveBeenCalledTimes(0);
expect(gd._fullData[0]._module.plot).toHaveBeenCalledTimes(0);
}
};
}
Plotly.plot(gd, mock)
.then(_assert('base', ['1', '1', '0.5', '0.5', '1', '1']))
.then(function() {
spyOn(Plots, 'doCalcdata').and.callThrough();
spyOn(gd._fullData[0]._module, 'plot').and.callThrough();
})
.then(_restyle({'marker.opacitybase': 0.2}))
.then(_assert('lower marker.opacitybase', ['1', '1', '0.2', '0.5', '1', '1']))
.then(_restyle({'marker.opacitybase': 0.8}))
.then(_assert('raise marker.opacitybase', ['1', '1', '0.8', '0.1', '0.2', '1']))
.then(_restyle({'marker.opacitybase': null}))
.then(_assert('back to dflt', ['1', '1', '0.5', '0.1', '0.2', '1']))
.catch(failTest)
.then(done);
});

  • That one can be simply 🔪 (you could add a restyle test for marker.depthfade, but that's 100% not required)

it('should be able to restyle *pathbar.opacity*', function(done) {
var mock = {
data: [{
type: 'treemap',
labels: ['Root', 'A', 'B', 'b', 'b2', 'b3'],
parents: ['', 'Root', 'Root', 'B', 'b', 'b2'],
level: 'b'
}]
};
function _assert(msg, exp) {
return function() {
var layer = d3.select(gd).select('.treemaplayer');
var opacities = [];
layer.selectAll('path.surface').each(function() {
opacities.push(this.style.opacity);
});
expect(opacities).toEqual(exp, msg);
// editType:style
if(msg !== 'base') {
expect(Plots.doCalcdata).toHaveBeenCalledTimes(0);
expect(gd._fullData[0]._module.plot).toHaveBeenCalledTimes(0);
}
};
}
Plotly.plot(gd, mock)
.then(_assert('base', ['0.5', '0.5', '1', '0.5', '0.5']))
.then(function() {
spyOn(Plots, 'doCalcdata').and.callThrough();
spyOn(gd._fullData[0]._module, 'plot').and.callThrough();
})
.then(_restyle({'pathbar.opacity': 0.2}))
.then(_assert('lower pathbar.opacity', ['0.5', '0.5', '1', '0.2', '0.2']))
.then(_restyle({'pathbar.opacity': 0.8}))
.then(_assert('raise pathbar.opacity', ['0.5', '0.5', '1', '0.8', '0.8']))
.then(_restyle({'pathbar.opacity': null}))
.then(_assert('back to dflt', ['0.5', '0.5', '1', '0.5', '0.5']))
.catch(failTest)
.then(done);
});

  • I'm not 100% sure what's going here, but I think we simply need to adjust the assertion values

@etpinard
Copy link
Contributor

etpinard commented Oct 4, 2019

One more thing:

  • make sure pathbar.opacity is 1 by default

@archmoj
Copy link
Contributor Author

archmoj commented Oct 4, 2019

One more thing:

  • make sure pathbar.opacity is 1 by default

It is 1 when a colorscale is present and 0.5 otherwise.
Is that OK?

@etpinard
Copy link
Contributor

etpinard commented Oct 4, 2019

It is 1 when a colorscale is present and 0.5 otherwise.
Is that OK?

I don't have a strong opinion about it, but looks like @nicolaskruchten would prefer 1 always.

I reran https://codepen.io/MojtabaSamimi/pen/poozNQx using this build and the brown went away, which is nice, but I'm not sure why the pathbar is so faded?
actually it was faded before, and I only now noticed it... if I set a marker.color to red I would expect the pathbar element to be fully red and not faded, no?

@archmoj
Copy link
Contributor Author

archmoj commented Oct 4, 2019

It is 1 when a colorscale is present and 0.5 otherwise.
Is that OK?

I don't have a strong opinion about it, but looks like @nicolaskruchten would prefer 1 always.

I reran https://codepen.io/MojtabaSamimi/pen/poozNQx using this build and the brown went away, which is nice, but I'm not sure why the pathbar is so faded?
actually it was faded before, and I only now noticed it... if I set a marker.color to red I would expect the pathbar element to be fully red and not faded, no?

OK. Let's go with that. Actually it would work better for text transitions.

@etpinard
Copy link
Contributor

etpinard commented Oct 7, 2019

@archmoj I'm not sure I understand this mock:

image

Why do the marker.depthfade: 'reversed' traces show color gradient while the marker.depthfade: true don't?

@etpinard
Copy link
Contributor

etpinard commented Oct 7, 2019

Moreover, in

image

why does the root node show up in white now?

@archmoj
Copy link
Contributor Author

archmoj commented Oct 7, 2019

@archmoj I'm not sure I understand this mock:

image

Why do the marker.depthfade: 'reversed' traces show color gradient while the marker.depthfade: true don't?

When depthfade is true the leaves are fully saturated. Since there is no space for headers (pad = 0), there is no gradient displayed on the figures on the right.

@archmoj
Copy link
Contributor Author

archmoj commented Oct 7, 2019

Moreover, in

image

why does the root node show up in white now?

To display coloured root cases the root should be drawn.
This is now fixed by 1dff468.

} else {
if(helpers.isHierarchyRoot(pt)) {
if(isRoot && fillColor === 'rgba(0,0,0,0)') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This solution correctly handle the default root case that sets cdi.color to rgba(0,0,0,0) here:

// root gets no coloring by default
cdi.color = 'rgba(0,0,0,0)';

but, we'll probably need to extend it all other rgba colors with a 0 alpha channel - as the current depthfade algorithm gives a white fillColor.

@etpinard
Copy link
Contributor

etpinard commented Oct 7, 2019

All right. Time to merge this thing 💃


@archmoj please keep in mind #4242 (review) for future treemap work.

@archmoj
Copy link
Contributor Author

archmoj commented Oct 7, 2019

@etpinard Thanks very much for allocating enough time for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants