react icon indicating copy to clipboard operation
react copied to clipboard

ButtonGroup styling is not compatible with new IconButton tooltips

Open camchenry opened this issue 1 year ago • 2 comments

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 avatar Jul 29 '24 16:07 camchenry

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

camchenry avatar Jul 29 '24 16:07 camchenry

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!

lesliecdubs avatar Aug 05 '24 15:08 lesliecdubs

Fixed with https://github.com/primer/react/pull/5123

langermank avatar Dec 19 '24 15:12 langermank

So happy that it is finally fixed 🥰 Thanks Katie!!

broccolinisoup avatar Dec 20 '24 01:12 broccolinisoup