Skip to content

Set enabled:false when filter target array is empty #3766

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
May 8, 2019
Merged

Set enabled:false when filter target array is empty #3766

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
66645ed
set enabled:false when filter target array is empty
etpinard Apr 12, 2019
1e57928
Merge branch 'master' into filter-enabled-false-when-empty-target-array
etpinard Apr 26, 2019
3a5a4c2
treat trace w/ _length===0 as if they were visible:false
etpinard Apr 26, 2019
afad530
fixup tests post _length===0 commit
etpinard Apr 26, 2019
b643238
get filter transform to work with scattercarpet traces
etpinard Apr 26, 2019
40c8abc
don't try to cleanDate coming from autorange
etpinard Apr 26, 2019
a20cb01
Merge branch 'master' into filter-enabled-false-when-empty-target-array
etpinard May 8, 2019
84c3606
fixup funnel test for new _length===0 logic
etpinard May 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/transforms/filter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,16 @@ exports.supplyDefaults = function(transformIn) {
var enabled = coerce('enabled');

if(enabled) {
var target = coerce('target');

if(Lib.isArrayOrTypedArray(target) && target.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@etpinard Thanks for the PR.
Cool!
What about handling a case like this: [ [ ] ]?
See codepen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Thanks for the review!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@archmoj this turned out to be a bit more of a project than anticipated.

See 3a5a4c2 - which makes things work for all trace types except scattercarpet and in some world-calendar scenario. I'll try to get those two cases fixed before 1.48.0, but in the meantime here's "a good chuck" of the solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which makes things work for all trace types except scattercarpet and in some world-calendar scenario.

Now fixed by b643238 and 40c8abc

Making this PR ready for a second 👁️

transformOut.enabled = false;
return transformOut;
}

coerce('preservegaps');
coerce('operation');
coerce('value');
coerce('target');

var handleCalendarDefaults = Registry.getComponentMethod('calendars', 'handleDefaults');
handleCalendarDefaults(transformIn, transformOut, 'valuecalendar', null);
Expand Down
27 changes: 27 additions & 0 deletions test/jasmine/tests/transform_filter_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,33 @@ describe('filter transforms defaults:', function() {
expect(traceOut.transforms[2].target).toEqual('x');
expect(traceOut.transforms[3].target).toEqual('marker.color');
});

it('supplyTraceDefaults should set *enabled:false* and return early when *target* is an empty array', function() {
// see https://github.com/plotly/plotly.js/issues/2908
// this solves multiple problems downstream

traceIn = {
x: [1, 2, 3],
transforms: [{
type: 'filter',
target: []
}]
};
traceOut = Plots.supplyTraceDefaults(traceIn, {type: 'scatter'}, 0, fullLayout);
expect(traceOut.transforms[0].target).toEqual([]);
expect(traceOut.transforms[0].enabled).toBe(false, 'set to false!');

traceIn = {
x: new Float32Array([1, 2, 3]),
transforms: [{
type: 'filter',
target: new Float32Array()
}]
};
traceOut = Plots.supplyTraceDefaults(traceIn, {type: 'scatter'}, 0, fullLayout);
expect(traceOut.transforms[0].target).toEqual(new Float32Array());
expect(traceOut.transforms[0].enabled).toBe(false, 'set to false!');
});
});

describe('filter transforms calc:', function() {
Expand Down