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

Handle case of div with zero dimension when positioning unified hover box #5913

Merged
merged 14 commits into from
Aug 31, 2021

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Aug 30, 2021

Fix to bring back unified hover in documentation website.
Regression introduced by #5846 in v2.3.0.

@plotly/plotly_js

@archmoj archmoj added bug something broken status: reviewable regression this used to work labels Aug 30, 2021
@alexcjohnson
Copy link
Collaborator

TBH I suspect we could get away without using getBoundingClientRect at all here - and that would avoid some bugs we probably have right now in case the user has the graph inside some sort of transform.

But in the meantime, to be consistent in our usage, couldn't we switch from outerContainer: fullLayout._paperdiv to outerContainer: fullLayout._paper.node() like we have everywhere else this gets used - ie use _paper (the main <svg>, that always seems to have a meaningful size) rather than _paperdiv (its enclosing <div>, that for whatever weird CSS reason sometimes has no size)... also just pass around a node, don't d3.select it here only to .node() it here

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃

@archmoj archmoj merged commit ba14be2 into master Aug 31, 2021
@archmoj archmoj deleted the unified-hover-with-zero-outer-height branch August 31, 2021 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken regression this used to work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants