Skip to content

Fix text position in centered tooltips #2154 #2407

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

Closed
wants to merge 1 commit into from

Conversation

rmoestl
Copy link
Contributor

@rmoestl rmoestl commented Feb 27, 2018

Suggested fix for #2154. Added test mock seems to cover #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 #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.
@rmoestl rmoestl closed this Feb 27, 2018
@rmoestl
Copy link
Contributor Author

rmoestl commented Feb 27, 2018

Accidentally created pull request in plotly.js main repository.

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.

1 participant