Skip to content

Conversation

@rebeccaalpert
Copy link
Member

I added an optional delay prop to the Tooltip component to specify the entrance delay. The external tooltip library defaults to an entrance delay of 0 ms.

The null in {[delay, null]} gets the external tooltip library to use its default exit delay (20 ms). Controlling the exit delay wasn't mentioned in the issue, so I didn't add a prop for it. Let me know if you want an extra prop for that.

I didn't make any changes to the overrides; my IDE removed the spaces at the ends of the lines. I can set it back the way it was if you want. Let me know.

Fixes #1161.

@coveralls
Copy link

coveralls commented Jan 16, 2019

Pull Request Test Coverage Report for Build 4289

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage remained the same at 80.225%

Files with Coverage Reduction New Missed Lines %
packages/patternfly-4/react-core/src/components/Tooltip/Tooltip.js 1 58.33%
Totals Coverage Status
Change from base Build 4288: 0.0%
Covered Lines: 4632
Relevant Lines: 5424

💛 - Coveralls

@TimoStaudinger
Copy link

TimoStaudinger commented Jan 17, 2019

Nielson Norman has published some pretty interesting guidelines on timing of exposing hidden content like tooltips. This research was one of the reasons this discussion originally popped up in our team.


They suggest both entry and exit delays in the 300 to 500 ms range:

For hover interactions, the timing guidelines for speed of visual feedback and exposing hidden elements must be broken down into more steps:

  1. Mouse cursor enters target area: display visual feedback within 0.1 seconds.
  2. Wait 0.3–0.5 seconds.
  3. If cursor remains stopped within target area, display corresponding hidden content within 0.1 seconds.
  4. Keep displaying the exposed content element until the cursor has left the triggering target area or the exposed content for longer than 0.5 seconds.

Their main argument is detecting intent: Tooltips should not show up if the user just happens to move the cursor over the tigger element, but only if the user actually intends to activate it -- otherwise, the tooltips will flicker into view for a sub-second, which may be distracting to the user. This example shows pretty well what I mean and what we're trying to avoid in our app if you move the mouse over the list fairly quickly.

On the other hand, tooltips should not disappear instantly instantly when the user moves the cursor off the trigger element, to make the tooltip more resilient against brief exits and re-enters of the cursor, and to make the UX less 'hectic' -- arguably, the exit delay is more relevant e.g. for hover menus, but it seems still valid to me.


What I'm taking from this discussion are two points:

  1. An exit delay prop may be a very useful addition, especially since it's already supported by tippy.js.
  2. Maybe a value in the range of 300 to 500ms for both entry and exit delay is a sensible default, rather than a default of 0ms.

I'm curious to hear what your thoughts are on this topic?

@jschuler
Copy link
Collaborator

I think it will be great to have entryDelay and exitDelay!

Copy link
Contributor

@dlabaj dlabaj left a comment

Choose a reason for hiding this comment

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

Think 500 ms is a good delay for default.

@rebeccaalpert
Copy link
Member Author

I added entryDelay and exitDelay props that default to 500 ms.

@patternfly-build
Copy link
Collaborator

PatternFly-React preview: https://1205-pr-patternfly-react-patternfly.surge.sh

tlabaj
tlabaj previously approved these changes Jan 28, 2019
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

dlabaj
dlabaj previously approved these changes Jan 28, 2019
Copy link
Collaborator

@jschuler jschuler left a comment

Choose a reason for hiding this comment

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

Can we wait on #1244 first to be resolved then we can check this again? Because otherwise it doesn't look quite right

@rebeccaalpert rebeccaalpert dismissed stale reviews from dlabaj and tlabaj via 37354e5 February 4, 2019 18:06
@rebeccaalpert
Copy link
Member Author

I rebased. It looks like #1244 has been merged. Can you please review?

@tlabaj tlabaj dismissed jschuler’s stale review February 4, 2019 21:57

#1244 has been merged

@tlabaj tlabaj merged commit 0c5e910 into patternfly:master Feb 4, 2019
@rebeccaalpert rebeccaalpert deleted the tooltip-delay branch February 4, 2019 22:05
dgutride pushed a commit to dgutride/patternfly-react that referenced this pull request Feb 6, 2019
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.

7 participants