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

gl3d reversescale fixup #3418

Merged
merged 4 commits into from
Jan 8, 2019
Merged

gl3d reversescale fixup #3418

merged 4 commits into from
Jan 8, 2019

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Jan 8, 2019

fixes #3417 - which slipped through our test mocks during #3341. This patch will be part of today's 1.43.2

cc @plotly/plotly_js - please note that gl3d traces don't use the same colorscale logic as the other traces. It makes use of colormap. When we'll rewrite gl3d in regl, we should move away from colormap and make all our trace type use the same colorscale logic.

@etpinard etpinard added bug something broken status: reviewable labels Jan 8, 2019
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.

Thanks @etpinard for the great fix.
Please find my questions below.

0.08865606199840186,
0.1693927420185106
],
"reversescale": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if the user also added "reversescl": false here it would overwrite "reversescale" attribute.

So you may consider adding those conditions here:

trace.reversescale = trace.reversescl;

For example:

// scl->scale, reversescl->reversescale
if('scl' in trace) {
    if(!'colorscale' in trace) trace.colorscale = trace.scl;
    delete trace.scl;
}
if('reversescl' in trace) {
    if(!'reversescale' in trace) trace.reversescale = trace.reversescl;
    delete trace.reversescl;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could add a test in the cleanData block

describe('cleanData & cleanLayout', function() {
var gd;
beforeEach(function() {
gd = createGraphDiv();
});
afterEach(destroyGraphDiv);
it('should rename \'YIGnBu\' colorscales YlGnBu (2dMap case)', function() {
var data = [{
type: 'heatmap',
colorscale: 'YIGnBu'
}];
Plotly.plot(gd, data);
expect(gd.data[0].colorscale).toBe('YlGnBu');
});
it('should rename \'YIGnBu\' colorscales YlGnBu (markerColorscale case)', function() {
var data = [{
type: 'scattergeo',
marker: { colorscale: 'YIGnBu' }
}];
Plotly.plot(gd, data);
expect(gd.data[0].marker.colorscale).toBe('YlGnBu');
});
it('should rename \'YIOrRd\' colorscales YlOrRd (2dMap case)', function() {
var data = [{
type: 'contour',
colorscale: 'YIOrRd'
}];
Plotly.plot(gd, data);
expect(gd.data[0].colorscale).toBe('YlOrRd');
});
it('should rename \'YIOrRd\' colorscales YlOrRd (markerColorscale case)', function() {
var data = [{
type: 'scattergeo',
marker: { colorscale: 'YIOrRd' }
}];
Plotly.plot(gd, data);
expect(gd.data[0].marker.colorscale).toBe('YlOrRd');
});
it('should rename \'highlightColor\' to \'highlightcolor\')', function() {
var data = [{
type: 'surface',
contours: {
x: { highlightColor: 'red' },
y: { highlightcolor: 'blue' }
}
}, {
type: 'surface'
}, {
type: 'surface',
contours: false
}, {
type: 'surface',
contours: {
stuff: {},
x: false,
y: []
}
}];
spyOn(Plots.subplotsRegistry.gl3d, 'plot');
Plotly.plot(gd, data);
expect(Plots.subplotsRegistry.gl3d.plot).toHaveBeenCalled();
var contours = gd.data[0].contours;
expect(contours.x.highlightColor).toBeUndefined();
expect(contours.x.highlightcolor).toEqual('red');
expect(contours.y.highlightcolor).toEqual('blue');
expect(contours.z).toBeUndefined();
expect(gd.data[1].contours).toBeUndefined();
expect(gd.data[2].contours).toBe(false);
expect(gd.data[3].contours).toEqual({ stuff: {}, x: false, y: [] });
});
it('should rename \'highlightWidth\' to \'highlightwidth\')', function() {
var data = [{
type: 'surface',
contours: {
z: { highlightwidth: 'red' },
y: { highlightWidth: 'blue' }
}
}, {
type: 'surface'
}];
spyOn(Plots.subplotsRegistry.gl3d, 'plot');
Plotly.plot(gd, data);
expect(Plots.subplotsRegistry.gl3d.plot).toHaveBeenCalled();
var contours = gd.data[0].contours;
expect(contours.x).toBeUndefined();
expect(contours.y.highlightwidth).toEqual('blue');
expect(contours.z.highlightWidth).toBeUndefined();
expect(contours.z.highlightwidth).toEqual('red');
expect(gd.data[1].contours).toBeUndefined();
});
it('should rename *filtersrc* to *target* in filter transforms', function() {
var data = [{
transforms: [{
type: 'filter',
filtersrc: 'y'
}, {
type: 'filter',
operation: '<'
}]
}, {
transforms: [{
type: 'filter',
target: 'y'
}]
}];
Plotly.plot(gd, data);
var trace0 = gd.data[0];
var trace1 = gd.data[1];
expect(trace0.transforms.length).toEqual(2);
expect(trace0.transforms[0].filtersrc).toBeUndefined();
expect(trace0.transforms[0].target).toEqual('y');
expect(trace1.transforms.length).toEqual(1);
expect(trace1.transforms[0].target).toEqual('y');
});
it('should rename *calendar* to *valuecalendar* in filter transforms', function() {
var data = [{
transforms: [{
type: 'filter',
target: 'y',
calendar: 'hebrew'
}, {
type: 'filter',
operation: '<'
}]
}, {
transforms: [{
type: 'filter',
valuecalendar: 'jalali'
}]
}];
Plotly.plot(gd, data);
var trace0 = gd.data[0];
var trace1 = gd.data[1];
expect(trace0.transforms.length).toEqual(2);
expect(trace0.transforms[0].calendar).toBeUndefined();
expect(trace0.transforms[0].valuecalendar).toEqual('hebrew');
expect(trace1.transforms.length).toEqual(1);
expect(trace1.transforms[0].valuecalendar).toEqual('jalali');
});
it('should cleanup annotations / shapes refs', function() {
var data = [{}];
var layout = {
annotations: [
{ ref: 'paper' },
null,
{ xref: 'x02', yref: 'y1' }
],
shapes: [
{ xref: 'y', yref: 'x' },
null,
{ xref: 'x03', yref: 'y1' }
]
};
Plotly.plot(gd, data, layout);
expect(gd.layout.annotations[0]).toEqual({ xref: 'paper', yref: 'paper' });
expect(gd.layout.annotations[1]).toEqual(null);
expect(gd.layout.annotations[2]).toEqual({ xref: 'x2', yref: 'y' });
expect(gd.layout.shapes[0].xref).toBeUndefined();
expect(gd.layout.shapes[0].yref).toBeUndefined();
expect(gd.layout.shapes[1]).toEqual(null);
expect(gd.layout.shapes[2].xref).toEqual('x3');
expect(gd.layout.shapes[2].yref).toEqual('y');
});
it('removes direction names and showlegend from finance traces', function() {
var data = [{
type: 'ohlc', open: [1], high: [3], low: [0], close: [2],
increasing: {
showlegend: true,
name: 'Yeti goes up'
},
decreasing: {
showlegend: 'legendonly',
name: 'Yeti goes down'
},
name: 'Snowman'
}, {
type: 'candlestick', open: [1], high: [3], low: [0], close: [2],
increasing: {
name: 'Bigfoot'
},
decreasing: {
showlegend: false,
name: 'Biggerfoot'
},
name: 'Nobody'
}, {
type: 'ohlc', open: [1], high: [3], low: [0], close: [2],
increasing: {
name: 'Batman'
},
decreasing: {
showlegend: true
},
name: 'Robin'
}, {
type: 'candlestick', open: [1], high: [3], low: [0], close: [2],
increasing: {
showlegend: false,
},
decreasing: {
name: 'Fred'
}
}, {
type: 'ohlc', open: [1], high: [3], low: [0], close: [2],
increasing: {
showlegend: false,
name: 'Gruyere heating up'
},
decreasing: {
showlegend: false,
name: 'Gruyere cooling off'
},
name: 'Emmenthaler'
}];
Plotly.plot(gd, data);
// Even if both showlegends are false, leave trace.showlegend out
// My rationale for this is that legends are sufficiently different
// now that it's worthwhile resetting their existence to default
gd.data.forEach(function(trace) {
expect(trace.increasing.name).toBeUndefined();
expect(trace.increasing.showlegend).toBeUndefined();
expect(trace.decreasing.name).toBeUndefined();
expect(trace.decreasing.showlegend).toBeUndefined();
});
// Both directions have names: ignore trace.name, as it
// had no effect on the output previously
// Ideally 'Yeti goes' would be smart enough to truncate
// at 'Yeti' but I don't see how to do that...
expect(gd.data[0].name).toBe('Yeti goes');
// One direction has empty or hidden name so use the other
// Note that even '' in both names would render trace.name impact-less
expect(gd.data[1].name).toBe('Bigfoot');
// One direction has a name but trace.name is there too:
// just use trace.name
expect(gd.data[2].name).toBe('Robin');
// No trace.name, only one direction name: use the direction name
expect(gd.data[3].name).toBe('Fred');
// both names exist but hidden from the legend: still look for common prefix
expect(gd.data[4].name).toBe('Gruyere');
});
});

at some point, but I won't do this in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Simply opened #3419 in this regard...

@archmoj
Copy link
Contributor

archmoj commented Jan 8, 2019

Thanks for the fix.
💃

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.

'reversescale' attribute: wrong behavior since v1.43.0
2 participants