-
Notifications
You must be signed in to change notification settings - Fork 116
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
Removing the button wrapper when using a tooltip with a button #2296
Conversation
🦋 Changeset detectedLatest commit: 3802727 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
6d77854
to
3eb29d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
Reviewers: By approving this Pull Request you are approving the code change, as well as its deployment and mitigation plans.
Please read this description carefully. If you feel there is anything unclear or missing, please ask for updates.
What are you trying to accomplish?
Remove the
withTooltip
wrapper that we add to buttons when they have a tooltip.The wrapper was initially added https://github.com/github/primer/issues/826 because of elements using different positioning the tooltip would end up in an incorrect place. However since this issue was occurring we have refactored tooltip to use the
popover
browser api #2111 which takes care of positioning problems.Screenshots
There shouldn't be any visual changes. Some of our components the spacing slightly changed because we're not dealing with the double dom elements anymore. I would consider the old state broken and the new images as fixed.
Integration
There is some code changes, the
wrapper_arguments
was removed, anything in these arguments should be moved to the main system arguments of the component.Example:
We should send this PR only to review-lab to make sure we don't see any regressions.
Risk Assessment
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.