Skip to content

Adding optional tooltip to ButtonSelect component #591

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

Merged
merged 7 commits into from
Aug 3, 2022

Conversation

alex501212
Copy link
Contributor

Description of proposed changes

Added a new optional tooltip to the ButtonSelect component
Updated design system site to reflect changes

Screenshot of proposed changes

Screenshot 2022-07-28 at 3 30 36 pm

Screenshot 2022-07-28 at 3 30 45 pm

@alex501212 alex501212 requested a review from a team as a code owner July 28, 2022 14:34
@alex501212 alex501212 requested a review from mardotio July 28, 2022 15:15
@mardotio
Copy link
Contributor

@alex501212 Any reason you can't just wrap ButtonSelect with TooltipHoverArea directly in your code?

@alex501212
Copy link
Contributor Author

@alex501212 Any reason you can't just wrap ButtonSelect with TooltipHoverArea directly in your code?

I tried doing this but wasn't able to get the tooltip to disappear upon opening the dropdown, so I thought this would be a good way to allow for that

@mardotio
Copy link
Contributor

mardotio commented Aug 1, 2022

Gotcha. In that case, could you make it so that the button is only wrapped with the tooltip component if the tooltip prop is passed in? That way the extra DOM elements associated with the tooltip aren't rendered if the tooltip isn't used.

Also, looks like some tests are failing.

@alex501212
Copy link
Contributor Author

Gotcha. In that case, could you make it so that the button is only wrapped with the tooltip component if the tooltip prop is passed in? That way the extra DOM elements associated with the tooltip aren't rendered if the tooltip isn't used.

Also, looks like some tests are failing.

Tooltip should now conditionally render, also got the tests passing

Copy link
Contributor

@mardotio mardotio left a comment

Choose a reason for hiding this comment

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

Sweet! Looks good.
@alex501212 can you just make sure to squash your commits before merging in order to avoid having a bunch of formatting/testing commits in the history.

@alex501212 alex501212 merged commit 16191ad into main Aug 3, 2022
@alex501212 alex501212 deleted the buttonSelectTooltip branch August 3, 2022 08:59
sean-mckenna added a commit that referenced this pull request Dec 6, 2024
* added tooltip to buttonSelect

* updated changelog and package version

* ran prettier

* test

* changed tooltip defaultprops

* added conditional render for tooltip

* updated failing snapshot
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.

2 participants