Skip to content

Conversation

@emplums
Copy link

@emplums emplums commented Mar 15, 2020

This PR fixes #711 - it looks like I added a z-index: 1 to focused buttons when I created the ButtonGroup component. As far as I can tell, this is a remnant of handling the button group css within the Button component and isn't necessary because we're setting the z-index of buttons inside ButtonGroup within the ButtonGroup styles.

It's fine to leave the z-index of the button elements inside of the ButtonGroup component since they are children of ButtonGroup and the entire group should sit wherever ButtonGroup is in the stacking context. I've double checked this with the test environment that @acknosyn provided in #711 reproducing the bug. Using ButtonGroup works fine in that test case.

Screenshots

Please provide before/after screenshots for any visual changes

Merge checklist

  • Added or updated TypeScript definitions (index.d.ts) if necessary
  • Added/updated tests
  • Added/updated documentation
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contibuting guidelines for more infomation on how we review PRs.

@vercel
Copy link

vercel bot commented Mar 15, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/primer/primer-components/604f5df2t
✅ Preview: https://primer-components-git-button-index.primer.now.sh

@vercel vercel bot temporarily deployed to Preview March 15, 2020 02:31 Inactive
@emplums emplums changed the title [WIP][FIX] Remove z-index on focused button [FIX] Remove z-index on focused button Mar 15, 2020
@emplums emplums added the minor release new features label Mar 15, 2020
@emplums emplums requested review from colebemis and dmarcey March 16, 2020 23:29
@vercel vercel bot temporarily deployed to Preview March 16, 2020 23:31 Inactive
Copy link
Contributor

@dmarcey dmarcey left a comment

Choose a reason for hiding this comment

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

I think that this is probably the correct change, but I think it will cause issues with the focus rect where there isn't enough margin around the button.

image

I think that the answer here is just add the correct margin, but just wanted to make sure that you were aware!

@emplums
Copy link
Author

emplums commented Mar 17, 2020

@dmarcey yep I'm aware! I think in cases where you want a button right up against another button, we'd use a Button group, otherwise if it's up against another element it should have appropriate margin to prevent this. The issue with adding z-index on our own is it can cause tons of bugs down the line if the user of the component isn't expecting a z-index to have been added! There's no z-index in Primer CSS for individual buttons so I think this is safe to do! :)

@emplums emplums changed the base branch from master to minor March 17, 2020 00:30
@emplums emplums merged commit f278e63 into minor Mar 17, 2020
@emplums emplums deleted the button-index branch September 16, 2020 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Button Component Z-Index and Click Events Issue in v13.2.0

3 participants