Skip to content

Add the ability to delay tooltip closing so they can be interacted with.#4364

Merged
MLoughry merged 5 commits intomicrosoft:masterfrom
wygoodin:wygoodin/tooltip_interaction
Apr 2, 2018
Merged

Add the ability to delay tooltip closing so they can be interacted with.#4364
MLoughry merged 5 commits intomicrosoft:masterfrom
wygoodin:wygoodin/tooltip_interaction

Conversation

@wygoodin
Copy link
Copy Markdown
Contributor

@wygoodin wygoodin commented Mar 26, 2018

Pull request checklist

  • Addresses an existing issue: Fixes previously discussed issue with tooltips
  • Include a change request file using $ npm run change

Description of changes

Adds closeDelay to tooltip hosts, enabling interaction with the given tooltip via a delay on closing.

asdf

@wygoodin wygoodin requested a review from micahgodbolt as a code owner March 26, 2018 17:49
@msftclas
Copy link
Copy Markdown

msftclas commented Mar 26, 2018

CLA assistant check
All CLA requirements met.

@wygoodin
Copy link
Copy Markdown
Contributor Author

Whoops, look like this is the wrong version, hold on on this for a bit

@wygoodin wygoodin closed this Mar 26, 2018
@wygoodin
Copy link
Copy Markdown
Contributor Author

Okay, fixed the issues

@wygoodin wygoodin reopened this Mar 26, 2018

this._closingTimer = window.setTimeout(() => {
this._toggleTooltip(false);
}, this.props.closeDelay);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you should use this._async here so that it's correctly cleared etc.

@wygoodin
Copy link
Copy Markdown
Contributor Author

I've fixed the requested change, is this PR good to go?


this._closingTimer = this._async.setTimeout(() => {
this._toggleTooltip(false);
}, this.props.closeDelay);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

duration is not optional and I don't see a default value for the prop, are we setting one?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The default is not to delay at all, and thus not have a duration. Use of the delay is therefore guarded.

}

private _clearDismissTimer = (): void => {
window.clearTimeout(this._closingTimer);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(thought I had commented on this before, somehow got lost..)

you should use the _async utility.

@MLoughry MLoughry merged commit e405fef into microsoft:master Apr 2, 2018
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants