Skip to content

Bug fix errorbars on bar trace may disappear after switch modebar to select and double click #3644

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 2 commits into from
May 23, 2019
Merged

Bug fix errorbars on bar trace may disappear after switch modebar to select and double click #3644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6519021
fix issue 3630 errorbars style after double click whitespace
archmoj Mar 16, 2019
5a5702f
adding a jasmine test to lock issue 3630
archmoj Mar 18, 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
2 changes: 2 additions & 0 deletions src/traces/bar/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ function styleOnSelect(gd, cd) {
stylePointsInSelectionMode(s, trace, gd);
} else {
stylePoints(s, trace, gd);

Registry.getComponentMethod('errorbars', 'style')(s);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. I'm not sure this is the correct fix. Look at https://codepen.io/etpinard/pen/gEKqgj?editors=1010 (the same as your before example, but with scatter traces). Selection doesn't dim the error bars.

Copy link
Contributor

Choose a reason for hiding this comment

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

Back in 1.36.0, selecting bars didn't dim the associated error bars: https://codepen.io/etpinard/pen/jJKdYN?editors=1000

We should probably eventually add a way to dim the error bars on selections (by adding a [un]selected.error_(x|y).color or by adding a trace-wide [un]selected.opacity), but that's for another day.

To fix this bug, I think we should make sure error bars on bar traces don't get dimmed on selection by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bar version is different from scatter possibly because in bar the select deals with both bar points and errorbar points. So I still think it may be a good patch to solve the regression.

Copy link
Contributor

Choose a reason for hiding this comment

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

.... and why should scatter and bar error bars behave differently?

Copy link
Contributor

Choose a reason for hiding this comment

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

... but yeah you're right that the regression is way worse than the inconsistency this fix brings up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.... and why should scatter and bar error bars behave differently?

In scatter within stylePoint function selectAll('path.point') is applied; whereas in bar within stylePoint function selectAll('path') is applied which resulted in errorbars being selected after click.

}
}

Expand Down
39 changes: 39 additions & 0 deletions test/jasmine/tests/select_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,45 @@ describe('Click-to-select', function() {
}
});

it('should maintain style of errorbars after double click cleared selection (bar case)', function(done) {
Plotly.newPlot(gd, { // Note: this call should be newPlot not plot
data: [{
x: [0, 1, 2],
y: [100, 200, 400],
type: 'bar',
marker: {
color: 'yellow'
},
error_y: {
type: 'sqrt'
}
}],
layout: {
dragmode: 'select'
}
})
.then(function() {
var x = 100;
var y = 100;
drag([[x, y], [x, y]]); // first empty drag
return doubleClick(x, y); // then double click
})
.then(function() {
assertSelectionCleared();
})
.then(function() {
d3.select(gd).select('g.plot').each(function() {
d3.select(this).selectAll('g.errorbar').selectAll('path').each(function() {
expect(d3.select(this).attr('style'))
.toBe('vector-effect: non-scaling-stroke; stroke-width: 2px; stroke: rgb(68, 68, 68); stroke-opacity: 1; opacity: 1; fill: rgb(255, 255, 0); fill-opacity: 1;', 'to be visible'
);
});
});
})
.catch(failTest)
.then(done);
});

describe('triggers \'plotly_selected\' before \'plotly_click\'', function() {
[
testCase('cartesian', require('@mocks/14.json'), 270, 160, [7]),
Expand Down