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

Fix layout of centered hover info #2154 #2440

Merged
merged 2 commits into from
Mar 5, 2018
Merged

Conversation

rmoestl
Copy link
Contributor

@rmoestl rmoestl commented Mar 5, 2018

This fixes #2154: when hover info (aka tooltip) is rendered in the center due to a lack of space left or right, the text overflowed the box and the secondary hove info (trace name) was hidden underneath the primary one.

Final solution:
2154_layout_centered-hover-info-final

Hint: part of the discussion took place in rmoestl#1.

- Bug: when a tooltip displays a sufficiently long text (also depends
  on overall plot dimensions), the tooltip isn't rendered right or
  left but centered. In this case the tooltip text was partly rendered
  outside the tooltip box and the secondary label (the trace name) was
  hidden underneath the primary label.
@rmoestl rmoestl self-assigned this Mar 5, 2018
@rmoestl rmoestl requested a review from alexcjohnson March 5, 2018 14:34
// diff of exactly 1 would fail the test
var tolerance = 1.1;
expect(elem1BB.top - elem2BB.top).toBeWithin(0, tolerance, msg);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Love these assert functions! They'd be good candidates for assets/custom_assertions but we can leave them here until someone else finds a use for them elsewhere.

assertElemInside(nodes.primaryText, nodes.primaryBox, 'Primary text inside box');
assertElemInside(nodes.secondaryText, nodes.secondaryBox, 'Secondary text inside box');

done();
Copy link
Collaborator

Choose a reason for hiding this comment

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

do these test cases need to be async? Can't we just take out done from the function args and here?

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 point. They don't need to be async. I'll go and change that.

@alexcjohnson
Copy link
Collaborator

@rmoestl looks great, beautiful tests. Just the one quick question #2440 (comment) and then I think we're ready to 💃

- No reason to run them asynchronously.
@rmoestl rmoestl merged commit 8731cb5 into master Mar 5, 2018
@etpinard etpinard added bug something broken status: reviewable labels Mar 5, 2018
@etpinard etpinard deleted the fix-centered-tooltip-layout branch March 5, 2018 15:45
@etpinard
Copy link
Contributor

etpinard commented Mar 5, 2018

Great PR @rmoestl !

Thanks very much 🎉

@alexcjohnson
Copy link
Collaborator

Can we close #865 now too?

@etpinard
Copy link
Contributor

etpinard commented Mar 5, 2018

@alexcjohnson unfortunately no,

https://codepen.io/eagor/pen/dXLNzX is still buggy. On the latest master, we get:

peek 2018-03-05 12-31

@alexcjohnson
Copy link
Collaborator

Interesting - looks like the text gets pushed up to avoid overlap with the orange bar, but the rect doesn't. @rmoestl since you've got your head wrapped around this part of the code, can you hunt this last bit down as well?

@rmoestl
Copy link
Contributor Author

rmoestl commented Mar 5, 2018

@alexcjohnson yes, of course.

In fact, I even thought about proposing this to be the next bug to fix. When I diagnosed #2154 and #865, I felt these were two different problems. But definitely in the same area of code, yes.

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.

Hover tooltip positioned wrong in stacked barchart when number of bar is 1
3 participants