primer / react

An implementation of GitHub's Primer Design System using React
https://primer.style/guides/react
MIT License
3.15k stars 536 forks source link

Odd Borders on `IconButtons` in `ButtonGroup` #3172

Closed mattcosta7 closed 1 year ago

mattcosta7 commented 1 year ago

Description

https://user-images.githubusercontent.com/8616962/232143686-d040c5e8-5cb3-459d-a98e-b7fbfbb16985.mov

In a button group, IconButtons seem to have the wrong borders on focus/hover

Steps to reproduce

         <ButtonGroup>
                <IconButton aria-label="" icon={GearIcon} />
                <IconButton aria-label="" icon={SortAscIcon} />
                <IconButton aria-label="" icon={SortAscIcon} />
              </ButtonGroup>

checkout hover borders

Version

latest

Browser

No response

broccolinisoup commented 1 year ago

Hi 👋 I think this issue might be related to https://github.com/primer/react/pull/2933? As far as I understand this behaviour was intentional, at least it was a tradeoff between the wiggly buttons vs border style https://github.com/primer/react/pull/2933 cc @langermank

mattcosta7 commented 1 year ago

Hi 👋 I think this issue might be related to #2933? As far as I understand this behaviour was intentional, at least it was a tradeoff between the wiggly buttons vs border style #2933 cc @langermank

Interesting. It definitely looks broken now, especially in dark mode.

Maybe we could set the border-left's color to the non-hover color when it's not the first button, so at least the left and right borders are balanced on 'central' buttons?

Something like -

Screenshot 2023-04-17 at 8 17 12 AM

instead of

Screenshot 2023-04-17 at 8 18 01 AM

Or potentailly something with display: inline-table and border-collapse? (not that I have tried it or thoughts on what that would break - just considering alternatives if possible)

mattcosta7 commented 1 year ago

^ the left border fix suggested above would also help out with this case where the focus outline and hover border on separately elements show, and it looks a little off (at least to me)

Screenshot 2023-04-17 at 9 10 12 AM
lesliecdubs commented 1 year ago

@langermank assigning you to this since you were mentioned above, but give a shout if you need some support!