-
Notifications
You must be signed in to change notification settings - Fork 377
fix(tooltip): change default entry and exit delay #5746
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
Conversation
|
PF4 preview: https://patternfly-react-pr-5746.surge.sh |
|
@redallen I tried it interactively now, I am not really happy with this change. I feel that if I was a user, hovering over a button, I would not leave my mouse there for more than a second to see that. In addition, In cockpit I can't think of any scenario where this change makes sense, so we would need to change the properties to the previous state. So, since the properties are exposed, the reporter of the issue can adjust them for their scenario. And I honestly don't see the severeness of the reported issue, if the user scrolls with the mouse, and tooltips get activated, they can move the mouse slightly to the side. |
|
@KKoukiou I've added the check. Are there cases other than buttons or icons where you'd expect no delay? |
KKoukiou
left a comment
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 will think about thoroughly tomorrow if there is no need for this to be merged today.
|
|
||
| function isButtonOrIcon(children: React.ReactNode) { | ||
| if (React.isValidElement(children)) { | ||
| if (['button', Button as any, 'svg', 'g'].includes(children.type)) { |
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.
Checkbox, Radio we also use tooltips to 'explain' why elements are disabled.
So when we disable a checkbox, we always wrap it with a tooltip and say 'Please blabla to enable this feature'
kmcfaul
left a comment
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.
Would this be considering breaking as the default values are changing?
tlabaj
left a comment
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.
@redallen I agree with Katie. I believe this is a breaking change. I would be ok changing the defaults for the beta components only.
|
When would this lead to unexpected behavior @tlabaj or @kmcfaul ? I consider it a bug that PatternFly tooltips show up instantly when in most operating systems and JS frameworks they have some delay. This leads to weird scenarios where pages can show more than one tooltip. If you can think of cases where you expect a no-delay tooltip I can add them to the |
|
After discussing further with @redallen @tlabaj and @kmcfaul we have decided to set the default delay before showing a tooltip on hover to be 500ms. As was stated above, this is expected behavior for tooltips and is consistent with what is seen for most web applications and operating systems. The initial delay of 1 second (1000ms) was probably too long so at half a second it's enough time to sense that the user has stopped mouse movement over an element while preventing tootips from firing as the user is just moving the mouse over the screen. While I recognize that this is technically breaking I view the current behavior as an oversight vs intended. |
7e63c37 to
792d3b8
Compare
mcarrano
left a comment
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.
Looks great. Thanks @redallen !
|
Your changes have been released in:
Thanks for your contribution! 🎉 |

What: Closes #5745
Additional issues: