Introduce follow through behavior for tooltips #13095

Merged
merged 3 commits into from Nov 2, 2016

Conversation

Projects
None yet
2 participants
@vjeux
Contributor

vjeux commented Oct 29, 2016

Inside of Nuclide, we have multiple places where we have multiple icons close together that have a tooltip: the left toolbar, the bottom status bar, the debugger icons...

The current behavior is very frustrating as you've got to wait for the delay on every single one of them. But, we have a clear signal that the user wants a tooltip when s/he waits the time to see it and it's likely that moving the mouse over the next item quickly means that s/he wants to see it as well.

This pull request introduces the concept of follow through: if you have seen a tooltip, you're going to instantly see tooltip on the next element you mouse over within a short timer (300ms right now).

Test Plan:
Video before:

Video after:

Released under CC0

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Oct 29, 2016

Contributor
Contributor

vjeux commented Oct 29, 2016

@@ -21,7 +23,7 @@ var Tooltip = function (element, options, viewRegistry) {
Tooltip.VERSION = '3.3.5'
-Tooltip.TRANSITION_DURATION = 150

This comment has been minimized.

@vjeux

vjeux Oct 29, 2016

Contributor

This variable wasn't used. It was there since the initial version

@vjeux

vjeux Oct 29, 2016

Contributor

This variable wasn't used. It was there since the initial version

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 29, 2016

Contributor

Awesome idea. Are you cool writing some tests for this?

Contributor

nathansobo commented Oct 29, 2016

Awesome idea. Are you cool writing some tests for this?

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Oct 29, 2016

Contributor

Absolutely, let me work on tests. I wasn't sure if you'd be interested in it so didn't want to spend the time upfront. Working on it :)

Contributor

vjeux commented Oct 29, 2016

Absolutely, let me work on tests. I wasn't sure if you'd be interested in it so didn't want to spend the time upfront. Working on it :)

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Oct 29, 2016

Contributor

I was checking Sublime to see how it handled this and it seems to implement the same concept:

Contributor

vjeux commented Oct 29, 2016

I was checking Sublime to see how it handled this and it seems to implement the same concept:

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 29, 2016

Contributor

Yeah it's going to be great. Sorry for this crazy jQuery-derived code in the tooltip system. It's pretty bad.

Contributor

nathansobo commented Oct 29, 2016

Yeah it's going to be great. Sorry for this crazy jQuery-derived code in the tooltip system. It's pretty bad.

Introduce follow through behavior for tooltips
Inside of Nuclide, we have multiple places where we have multiple icons close together that have a tooltip: the left toolbar, the bottom status bar, the debugger icons...

The current behavior is very frustrating as you've got to wait for the delay on every single one of them. But, we have a clear signal that the user wants a tooltip when s/he waits the time to see it and it's likely that moving the mouse over the next item quickly means that s/he wants to see it as well.

This pull request introduces the concept of follow through: if you have seen a tooltip, you're going to instantly see tooltip on the next element you mouse over within a short timer (300ms right now).

Test Plan:
Video before:
![](http://g.recordit.co/7PCg0hjohP.gif)

Video after:
![](http://g.recordit.co/9OnZCvy9oI.gif)

Released under CC0
@@ -149,6 +160,29 @@ describe "TooltipManager", ->
it "hides the tooltips", ->
manager.add element, title: "Title"
hover element, ->
- expect(document.body.querySelector(".tooltip")).toBeDefined()
+ expect(document.body.querySelector(".tooltip")).not.toBeNull()

This comment has been minimized.

@vjeux

vjeux Nov 1, 2016

Contributor

I think it's better to write .not.toBeNull because .toBeDefined is .not.toBeUndefined which is also true if it's null.

@vjeux

vjeux Nov 1, 2016

Contributor

I think it's better to write .not.toBeNull because .toBeDefined is .not.toBeUndefined which is also true if it's null.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Nov 1, 2016

Contributor

Added a test that didn't pass before and now passes. Let me know if you want me to write more tests :)

Ran it with

atom --test spec/tooltip-manager-spec.coffee
Contributor

vjeux commented Nov 1, 2016

Added a test that didn't pass before and now passes. Let me know if you want me to write more tests :)

Ran it with

atom --test spec/tooltip-manager-spec.coffee
Relocate/rephrase tooltip follow-through test
Also make sure it doesn't leave tooltips on the DOM, which causes
subsequent tests to fail
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 1, 2016

Contributor

The test looks great. I relocated it to fit a bit better into the existing test structure and rephrased the description a bit to unpack what "follow-through" means to a newcomer to the code. Once this goes green we can 🚢. Nice addition! Thanks so much.

Contributor

nathansobo commented Nov 1, 2016

The test looks great. I relocated it to fit a bit better into the existing test structure and rephrased the description a bit to unpack what "follow-through" means to a newcomer to the code. Once this goes green we can 🚢. Nice addition! Thanks so much.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Nov 2, 2016

Contributor

The test failure on appveyor seems unrelated

Contributor

vjeux commented Nov 2, 2016

The test failure on appveyor seems unrelated

@nathansobo nathansobo merged commit 4ce0f5c into master Nov 2, 2016

2 of 5 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@nathansobo nathansobo deleted the fb-vj-follow-through branch Nov 2, 2016

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 2, 2016

Contributor

Thanks again!

Contributor

nathansobo commented Nov 2, 2016

Thanks again!

@simurai simurai referenced this pull request May 10, 2017

Closed

Tooltip "discovery" mode #5476

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