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

ButtonGroup styling is not compatible with new IconButton tooltips #4790

Open
camchenry opened this issue Jul 29, 2024 · 2 comments
Open

ButtonGroup styling is not compatible with new IconButton tooltips #4790

camchenry opened this issue Jul 29, 2024 · 2 comments
Assignees
Labels

Comments

@camchenry
Copy link
Contributor

Description

When wrapping a group of IconButton components with a ButtonGroup while using version 35.26.0 or higher, the styling of the button group and tooltips appears incorrect. Specifically:

  • Tooltips lose their border radius
  • The last button in the last does not have correct border radius

Here is an image displaying the issue:
Button group with a minus icon button and a plus icon button, currently focused. The plus icon has a tooltip of "Zoom in". The plus icon lacks any border radius, and the tooltip is only rounded on the right side.

  • The plus icon button should be rounded on the right side
  • The tooltips should always be fully rounded in each corner.

It looks like the issue stems from the fact that the button group is styling all descendents, which happens to include the new tooltip components, which are rendered as siblings of the buttons.

This might be fixed by updating the selector to exclude popovers:

/* Before */
&& > *:not([data-loading-wrapper]) { /* ... */ }

/* After (proposed) */
&& > *:not([data-loading-wrapper]):not([popover]) { /* ... */ }

Other fixes might be necessary, but I think this is the core issue.

Steps to reproduce

This issue can be seen in the Primer ButtonGroup documentation: https://primer.style/react/ButtonGroup#with-icon-buttons.

It can be reproduced by wrapping a group of labeled icon buttons with a ButtonGroup:

<ButtonGroup>
  <IconButton aria-label="Zoom out" icon={DashIcon} />
  <IconButton aria-label="Zoom in" icon={PlusIcon} />
</ButtonGroup>

Version

v35.26.0

Browser

No response

@camchenry camchenry added the bug Something isn't working label Jul 29, 2024
@camchenry
Copy link
Contributor Author

cc @broccolinisoup as this looks to be related to the new tooltip component, which is indirectly causing this issue in the button group. We had to work around this in Projects by redefining the button group styling to correctly account for the popover elements: https://github.com/github/github/blob/591030ae09c2a55e5b88a4542f912df8920baeb9/ui/packages/memex/src/client/components/top-bar/index.tsx#L290-L309

@lesliecdubs
Copy link
Member

Thanks for the report. @broccolinisoup putting this in your "up next" queue, but please let us know if this doesn't fit into your current work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants