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(positioning): make sure tooltip is oriented correctly when close to edge #391

Merged
merged 3 commits into from
Jun 7, 2018
Merged

fix(positioning): make sure tooltip is oriented correctly when close to edge #391

merged 3 commits into from
Jun 7, 2018

Conversation

gris-martin
Copy link
Contributor

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.

Fixes #371

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.
let outside = p => outsideLeft(p) || outsideRight(p) || outsideTop(p) || outsideBottom(p)
let inside = p => !outside(p)

let placesList = ['left', 'right', 'top', 'bottom']
Copy link
Collaborator

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)

Copy link
Collaborator

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.

Copy link
Contributor Author

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

@gris-martin gris-martin changed the title Make sure tooltip is oriented correctly when close to edge fix(positioning): make sure tooltip is oriented correctly when close to edge Jun 7, 2018
@aronhelser
Copy link
Collaborator

I was able to test, and it looks good. It also fixes the problem with entering from the top. Merging!

@aronhelser aronhelser merged commit 4fb4b40 into ReactTooltip:master Jun 7, 2018
@gris-martin gris-martin deleted the tooltip-orientation branch June 7, 2018 19:39
@TotallWAR
Copy link

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

@aronhelser
Copy link
Collaborator

I'm sorry, I don't understand. Can you provide an example or steps to reproduce the problem?

@aronhelser
Copy link
Collaborator

And potentially file an issue...

@TotallWAR
Copy link

image

As you can see, reposition works perfectly, however both of pseudo elements (for both posotions) are shown

@TotallWAR
Copy link

image

It's an example without reposition, when it works default.

@TotallWAR
Copy link

Any ideas?

@aronhelser
Copy link
Collaborator

🎉 This PR is included in version 3.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

3 participants