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

feat(tooltip): add support for hoverable definition tooltip body #5323

Merged

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Feb 12, 2020

Closes #3616

This PR adds support for hoverable definition tooltip body so that users can now select the text in the tooltip

Changelog

New

  • .bx--tooltip--visible class which is applied and removed (debounced) based on mouseenter, mouseleave, and Esc keydown events

Changed

  • focus event handler to focusin event so that tooltips are correctly restored after Esc keypress and subsequent focus

Removed

  • pointer-events: none rules

Testing / Reviewing

Ensure that definition tooltip body content is now selectable

@emyarod emyarod requested a review from a team as a code owner February 12, 2020 01:17
@ghost ghost requested review from dakahn and joshblack February 12, 2020 01:17
@netlify
Copy link

netlify bot commented Feb 12, 2020

Deploy preview for carbon-elements ready!

Built with commit e78642a

https://deploy-preview-5323--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Feb 12, 2020

Deploy preview for carbon-components-react ready!

Built with commit e78642a

https://deploy-preview-5323--carbon-components-react.netlify.com

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @emyarod!

@emyarod emyarod force-pushed the 3616-definition-tooltip-hoverable branch 2 times, most recently from 185f31a to 59249d0 Compare February 21, 2020 15:19
}
);
const debounceTooltipVisible = debounce(() => setTooltipVisible(false), 100);
Copy link
Contributor

@joshblack joshblack Feb 24, 2020

Choose a reason for hiding this comment

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

Do we need to debounce the exit? Are there any timing windows that we should be hitting for delays? Curious when we'd use 100/300/etc 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

the 100ms delay value is arbitrary but debouncing the exit is what resolves the original issue. afaik there is no guidance on the delay since the guidelines are still WIP w3c/aria-practices#127 w3c/aria-practices#128

@emyarod emyarod force-pushed the 3616-definition-tooltip-hoverable branch from ed84a6e to 851fa31 Compare February 25, 2020 17:08
@asudoh
Copy link
Contributor

asudoh commented Feb 25, 2020

Got a green light from @emyarod to move this to "ready to merge" state - Thanks @emyarod!

@asudoh asudoh merged commit 550bc70 into carbon-design-system:master Feb 25, 2020
@emyarod emyarod deleted the 3616-definition-tooltip-hoverable branch February 26, 2020 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Definition tooltip does not support Hoverable requirement
3 participants