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

scatter & bar hover fixes for #780 #2218

Merged
merged 1 commit into from Dec 21, 2017

Conversation

Projects
None yet
2 participants
@alexcjohnson
Copy link
Contributor

commented Dec 20, 2017

Fixes #780 - @etpinard another quick bugfix for you to look at.

Scatter hover was too permissive and too flat in its distance function in compare mode - so it would call the wrong point "closest" sometimes. That was the main culprit in #780, we were matching a scatter point with one x value to a bar with a different x and concluding that they really shouldn't "compare" with each other, so tossing out the bar.

Bar hover was also too restrictive, it would not allow you to be hovered on a bar if the mouse was over the gap between bars. Now in compare mode it allows that, which makes for a more pleasant (and useful) effect as you mouse over, particularly when the bars are very narrow: the hover labels don't flash on/off anymore, there's always one showing.

For both trace types I left "closest" hover behavior as it was. In particular this means the "flashing" effect for bars is still there in this mode. Perhaps there would be a way to fix it at least for very thin bars, by rounding outward or something in our acceptance criteria, but at least this doesn't seem to lead to any explicit bugs.

scatter & bar hover fixes for #780
scatter was too permissive and too flat in its distance function in compare mode
bar was too restrictive, in compare mode it now allows hover while in the gap between bars/groups
var rad = Math.max(3, di.mrc || 0);
return Math.max(Math.abs(xa.c2p(di.x) - xpx) - rad, 1 - 3 / rad);

This comment has been minimized.

Copy link
@etpinard

etpinard Dec 20, 2017

Member

I think I copied this piece of logic elsewhere e.g. in scattergeo/hover.js. Should we update it there too?

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Dec 21, 2017

Author Contributor

scattergeo should be fine, as it only uses the closest form (ie real quadrature distance, not just x or just y). I suppose the minRad change is still relevant there, to better handle line-only traces, but that's it.


afterEach(destroyGraphDiv);

it('shows hover info for both traces', function(done) {

This comment has been minimized.

Copy link
@etpinard

etpinard Dec 20, 2017

Member

Nice test 🔒

@@ -1274,7 +1311,7 @@ describe('hover updates', function() {

afterEach(destroyGraphDiv);

function assertLabelsCorrect(mousePos, labelPos, labelText) {
function assertLabelsCorrect(mousePos, labelPos, labelText, msg) {

This comment has been minimized.

Copy link
@etpinard

etpinard Dec 20, 2017

Member

Great. So none of the current tests needed to be updated, is that correct?

})
.then(unhover)
.then(function() {
expect(hoverCnt).toEqual(1);
expect(unHoverCnt).toEqual(1);

return assertLabelsCorrect([400, 200], [435, 198], '3');
return assertLabelsCorrect([420, 100], [435, 198], '3', 'events 1');

This comment has been minimized.

Copy link
@etpinard

etpinard Dec 21, 2017

Member

Ha. Found one. Why did this change?

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Dec 21, 2017

Author Contributor

x=400 is a little too far away using the new compare logic. I don't know exactly where the center of the marker is (that's why I've moved to more explicitly-sized-and-ranged plots for tests like this, so we can easily reason about the pixel positions of different points), probably the test would have worked just increasing to 405 or something but I just moved the mouse to a point I knew would be in range.

@etpinard

This comment has been minimized.

Copy link
Member

commented Dec 21, 2017

Looks great. Thanks for the clarifications. 💃

@alexcjohnson alexcjohnson merged commit 363180b into master Dec 21, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@alexcjohnson alexcjohnson deleted the scatter-bar-hover-fix branch Dec 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.