-
-
Notifications
You must be signed in to change notification settings - Fork 529
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(positioning): make sure tooltip is oriented correctly when close to edge #391
fix(positioning): make sure tooltip is oriented correctly when close to edge #391
Conversation
We now check different tooltip orientations to make sure the best one is chosen. There are 2 cases that lead to reorientation: 1. If the desired orientation (desiredPlace) is completely inside the client window, and the current orientation (place) is different from the desired one (then reorient to the desired orientation) 2. If neither the desired orientation (desiredPlace) nor the current one (place) is inside the client window, but there are other orientations that are (then reorient to an arbitrary orientation that is inside) All other cases will keep the orientation as it was before.
src/utils/getPosition.js
Outdated
let outside = p => outsideLeft(p) || outsideRight(p) || outsideTop(p) || outsideBottom(p) | ||
let inside = p => !outside(p) | ||
|
||
let placesList = ['left', 'right', 'top', 'bottom'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might want 'top' and 'bottom' to have priority - they will if you list them first, right?
I really like how you've simplified the code - it's much clearer. I still need to try it out (or someone does)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and please format the first line of you commit message for semantic versioning:
'fix(positioning): make sure ...'
If you do, travis will do an automatic release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the pull request title now, is that enough?
You're right about the orientation priority, it's changed with 6f01ed8
I was able to test, and it looks good. It also fixes the problem with entering from the top. Merging! |
There is i little bug here. Reposition works well but pseudo element breaks. It's shown both of pseudo elemnts, for instance: position was left but its located at the corner of window so its repositioned and after elements shown for both of positions |
I'm sorry, I don't understand. Can you provide an example or steps to reproduce the problem? |
And potentially file an issue... |
Any ideas? |
🎉 This PR is included in version 3.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
We now check different tooltip orientations to make sure the best one is chosen. There are 2 cases that lead to reorientation:
If the desired orientation (
desiredPlace
) is completely inside the client window, and the current orientation (place
) is different from the desired one (then reorient to the desired orientation)If neither the desired orientation (
desiredPlace
) nor the current one (place
) is inside the client window, but there are other orientations that are (then reorient to an arbitrary orientation that is inside)All other cases will keep the orientation as it was before.
Fixes #371