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 text position in centered tooltips #2154 #1

Closed
wants to merge 1 commit into from

Conversation

rmoestl
Copy link
Owner

@rmoestl rmoestl commented Feb 27, 2018

Suggested fix for plotly#2154. Added test mock seems to cover plotly#865 as well.

Regarding the fix:

  • Deleted the lines in alignHoverText because it fixes the scenario in question and because I wasn't able to figure out the intent of these four lines of code.
  • To figure out the intent, I also tracked down the origin of this piece of code: it was introduced in 2e69310. I studied the change set of this commit and its message, but wasn't able to understand the exact intent either.
  • I'm not entirely sure that the deleted lines are relevant for certain kinds of plots. I guess a more experienced contributer is able to make a better judge.

Regarding tests:

  • I added a mock that reproduces the issue and have noticed that for each mock there's an image as the rendered result. This image is missing in this commit, because I'm not sure how to run the image pixel tests described in image test README and trigger the required hover event.

Regarding related issue plotly#865:

  • Added test mock shows the problem when two tooltips overlap.

- 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.
@alexcjohnson
Copy link

Can you include a screenshot of what a centered label looks like now?

Just for reference: the idea is that hoverPoints routines return a label along with a box to position it outside of (x0, x1, y0, and y1). Then we first try to put it on the right side (ie pointing at x1 from the right), if that fails we try to point it at the left side of x0, and if BOTH of those fail, that's the situation here, where the box is too big to put the label on either side. Then the idea is to fall back on centering the hover label in the box, but showing all the same information.

"short label",
"Very long label named San Francisco",
"Very long label named San Francisco",
"Very long label named San Francisco"

Choose a reason for hiding this comment

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

Yep, this is one of the situations that trigger a centered label, when the text to be displayed is extra long. The other common one is a single-bar bar chart filling the whole plot area. I'm not sure we need a mock for this though, as image tests won't show hover labels - I think better would be to just generate these conditions as jasmine tests in https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/hover_label_test.js, and verify something about the label positioning/shape.

@rmoestl
Copy link
Owner Author

rmoestl commented Mar 2, 2018

I was able to add a Jasmine test.

However, while figuring out a minimal plot configuration to provoke the behavior, I think I got a better understanding what the problem is. The bug only occurs when there's more than one category (trace) which results in hover info displaying the trace name as the secondary text.

But when there's not enough space and hence hover info is centered, the display of the category name is positioned underneath the primary hover info element at the moment. Here's a screenshot to underpin my findings:
2154_buggy_centered-hover-info

trace1 is visible while trace0 is hidden underneath the centered, blue hover info box.

Which leads me to the assumption that the intent of suspect code is to position the secondary hover info (trace0) left (or right) to the primary one. txx is the x position of the primary text. tx2x the x position of the secondary one. The code in question I deleted in the first commit of this PR:

if(d.anchor === 'middle') {
    txx -= d.tx2width / 2;
    tx2x -= d.tx2width / 2;
}

Here's a screenshot of placing the secondary hover info left to the primary one:
2154_layout_centered-hover-info

Would this be a good solution?

rmoestl pushed a commit that referenced this pull request Jun 15, 2018
* treat zero as valid fixes plotly#2660

* PR suggestions: toBe instead of toEqual
rmoestl pushed a commit that referenced this pull request Jun 15, 2018
Fix for number 0 treated as invalid in hover (#1)
@rmoestl rmoestl closed this Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants