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

Speed up tooltips by default #640

Merged
merged 3 commits into from
Feb 10, 2017
Merged

Speed up tooltips by default #640

merged 3 commits into from
Feb 10, 2017

Conversation

llorca
Copy link
Contributor

@llorca llorca commented Feb 7, 2017

  • Sets hoverOpenDelay to 0
  • Keep transition duration at 100ms

Thoughts? cc @adidahiya @pkwi @thisisalessandro

@blueprint-bot
Copy link

Speed up tooltips by default

Preview: docs
Coverage: core | datetime

@leebyp
Copy link
Contributor

leebyp commented Feb 7, 2017

This becomes a problem once you start having tooltip in more complicated components (eg. tables and graphs) - granted those can reset the default themselves but I'm curious on the reasoning when a delay is also pretty standard.

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

before: comfortable delay is comfortable. tooltip-before

after: no delay is frightening.
tooltip-after

maybe try 50 or 100, but i really have no problem with 150. where did this change come from?

@sonovawolf
Copy link

Interesting 🤔 ours does feel a little slow but zero may be too snappy.

@pkwi
Copy link
Contributor

pkwi commented Feb 8, 2017

I'm happy with faster transitions but 0 delay doesn't work imo. You want them fast enough but also delayed enough so that you don't trigger them constantly as you move your mouse.
@llorca how about 50ms (or 100ms) for the delay and 50ms for the transition?

@@ -155,7 +155,7 @@ export class Tooltip extends React.Component<ITooltipProps, {}> {
interactionKind={PopoverInteractionKind.HOVER_TARGET_ONLY}
lazy={true}
popoverClassName={classes}
transitionDuration={200}
transitionDuration={transitionDuration}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Double check that this is correct

Copy link
Contributor

Choose a reason for hiding this comment

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

the better thing to do is to remove this line entirely cuz it's covered by {...this.props} spread

@llorca
Copy link
Contributor Author

llorca commented Feb 8, 2017

Hover delay 100ms
Animation 100ms

How's that?

@blueprint-bot
Copy link

Tweak open delay speed

Preview: docs
Coverage: core | datetime

@blueprint-bot
Copy link

Remove duplicate props

Preview: docs
Coverage: core | datetime

@adidahiya
Copy link
Contributor

I like 100/100

@giladgray giladgray merged commit f262131 into master Feb 10, 2017
@giladgray giladgray deleted the al/tooltip-default-speed branch February 10, 2017 19:42
@giladgray giladgray mentioned this pull request Feb 11, 2017
@noah79
Copy link

noah79 commented Mar 22, 2017

How about a way for us to set the default site-wide? I'm looking at having to wrap everywhere in my app with a version with a hardcoded default.

@giladgray
Copy link
Contributor

@noah79 sounds like a constant to me. we'd be happy to discuss such a feature but I make no promises, as we have no precedent for globally modifying defaults like this.

but wait! have you tried something like

Blueprint.Core.Tooltip.defaultProps.hoverOpenDelay = MY_MAGIC_VALUE;

@giladgray
Copy link
Contributor

^ simply modifying the default prop value (which is already a static constant) seems like the correct answer for your use case (though I've never tried modifying static component members like this).

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.

8 participants