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

Refactor EuiToolTip, add EuiPortal #484

Merged
merged 16 commits into from
Mar 9, 2018
Merged

Conversation

snide
Copy link
Contributor

@snide snide commented Mar 8, 2018

Closes #401, #71

Rewrites the tooltip set of components. Has the following changes.

  • Tooltips now work and are positioned correctly against the screen as room allows.
  • Tooltips can now have an optional clickOnly ability for when you want them to show/dismiss on click. This was removed for accessibility concerns.
  • Tooltips use a new utility component called EuiPortal which transports them to the bottom of the body where they are absolutely positioned (rather than fixed, so they are scrollable) against the body.
  • Renamed and reworked noOverFlowPlacement service to be calculatePopoverPosition and calculatePopoverStyles. They should now be generic enough to work with EuiPopover.
  • Tooltips are now accessible.

Todo

  • Figure out some state / repaint issues. Right now the onClick requires two clicks. Think this is due to the tooltip not getting the rect dimensions on the first draw.
  • Apply calculatePopoverPostion and calculatePopoverStyles to EuiPopover as well.
  • Reposition clicked tooltips when browser width is changed.
  • Clean up docs.

@snide snide requested a review from cjcenizal March 8, 2018 20:48
@snide
Copy link
Contributor Author

snide commented Mar 8, 2018

This is good to go on my end and I'm happy with the functionality and code. @cjcenizal is gonna see if he can apply the logic I'm using for calculating position of these to be used in the popovers as well. We might merge this in quicker if that looks like a bigger task.

@jen-huang you're welcome to check it out to make sure it works for what you need and make sure I didn't leave out any functionality.

@jen-huang
Copy link
Contributor

@snide Awesome work! Definitely works for what I need. Just noticed some small things while testing:

  1. Clicking outside of a clickOnly tooltip doesn't seem to dismiss it ("click or leave focus"). I need to click inside the tooltip or on its trigger:

mar-08-2018 16-17-03

  1. When I hover over the position="right" tooltip, it is positioned as expected. Then I resize my window to the point where there isn't any room on the right, then hover over the tooltip again, it is misplaced too high. On the second hover try, it is positioned correctly again:

mar-08-2018 16-18-45

@snide
Copy link
Contributor Author

snide commented Mar 9, 2018

@jen-huang I think I can add an click detector for the first one. The second one is gonna be tricky. Likely some sort of paint timing issue. I'll fool around.

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

This looks great man! Just had one comment.

@@ -0,0 +1,3 @@
.euiPortal {

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we delete this file?

@snide
Copy link
Contributor Author

snide commented Mar 9, 2018

jenkins, test this

1 similar comment
@snide
Copy link
Contributor Author

snide commented Mar 9, 2018

jenkins, test this

@snide snide merged commit ab8b98b into elastic:master Mar 9, 2018
@snide
Copy link
Contributor Author

snide commented Mar 9, 2018

Merged. @cjcenizal made the accessibility events work a little cleaner, but it meant we had to drop the clickOnly prop / feature. We can revisit later if we find we really need it. Messed with the repaint concerns to no avail, but since it's such a minor issue and I suspect trouble with the event listener and all the state changes happening here I'm gonna start a new issue to track it. Seemed like a big enough blocker for people we should get this working version in first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Tooltip positioning
4 participants