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: adds tooltip component #84

Merged
merged 4 commits into from May 22, 2023
Merged

feat: adds tooltip component #84

merged 4 commits into from May 22, 2023

Conversation

ryanhagerty
Copy link
Contributor

@ryanhagerty ryanhagerty commented Feb 3, 2023

Summary

This provides a tooltip component.

How to review this pull request

  • Check out the new tooltip component in Storybook
  • Click on the controls to change where the tooltip direction is coming from (see screenshot below). Note: There is a bug in storybook where changing a control doesn't update the JS. To get around this: once you change the control direction, click another story, click back on the tooltip story and now it will work.

Screenshots

Screen Shot 2023-02-03 at 8 49 59 AM

@ryanhagerty ryanhagerty added the 👍 Ready for Review Work is ready for review. label Feb 3, 2023
@joetower joetower self-requested a review March 10, 2023 15:26
@joetower joetower added 👁 Review in progress Under review. and removed 👍 Ready for Review Work is ready for review. labels Mar 10, 2023
Copy link
Contributor

@joetower joetower left a comment

Choose a reason for hiding this comment

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

@ryanhagerty This is great! I noticed a couple minor things that should probably be addressed before we approve and merge this.

  1. There is no :focus state on the tooltip when keyboard navigating. I realize this may be tricky, given how the button is only the i element. Perhaps this needs to be refactored a bit to allow for proper focus states?
  2. Also, the .tooltip__content is set to display: none and I'm wondering if we should change this to visibility: hidden or something else that is more accessible? I don't think a lot of screen readers would read the tooltip content as it is right now.

@joetower joetower added 👉 Needs Work Reviewer has requested changes. and removed 👁 Review in progress Under review. labels Mar 10, 2023
@ryanhagerty ryanhagerty added 👍 Ready for Review Work is ready for review. and removed 👉 Needs Work Reviewer has requested changes. labels May 4, 2023
@ryanhagerty
Copy link
Contributor Author

@joetower

Thanks for the review Joe.

The tooltip had a focus state, but it was so small. I bumped it up a bit. I also changed the visibility (thanks) and added an unrequested Esc key option to close the tooltip for accessiblity.

@joetower joetower self-requested a review May 5, 2023 17:13
Copy link
Contributor

@joetower joetower left a comment

Choose a reason for hiding this comment

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

@ryanhagerty This looks great! Thanks for making those changes.

@joetower joetower added 🎉 Passes Code Review Code is approved by the reviewer. 🎉 Passes Functional Review Functionality is approved by the reviewer. and removed 👍 Ready for Review Work is ready for review. labels May 5, 2023
@mcortes19 mcortes19 marked this pull request as draft May 22, 2023 17:35
@mcortes19 mcortes19 marked this pull request as ready for review May 22, 2023 17:36
@mcortes19 mcortes19 merged commit 4100852 into main May 22, 2023
2 checks passed
@github-actions
Copy link

🎉 This PR is included in version 1.15.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 Passes Code Review Code is approved by the reviewer. 🎉 Passes Functional Review Functionality is approved by the reviewer. released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants