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

Reduced spacing between buttons in a button group to 0.25 rem #511

Closed
wants to merge 1 commit into from
Closed

Reduced spacing between buttons in a button group to 0.25 rem #511

wants to merge 1 commit into from

Conversation

MadMango
Copy link

@MadMango MadMango commented Dec 11, 2023

Summary

Potentially fixes #504

Not sure if it should remain 0.5 rem for large buttons, happy to update this PR if so.

Also, I noticed that the buttonSize and buttonAs controls don't work in the storybook so I've created another pr for this here.

I couldn't get playwright to work reliably so I've not updated snapshots, but I think there's a pipeline workflow for this.

Not sure what I'm doing with changeset either so if it needs to look any different, please let me know 😄

List of notable changes:

Reduced spacing between buttons in a button group to 0.25 rem to match the designs

What should reviewers focus on?

Spacing between the buttons in a Button Group

Steps to test:

  1. Open storybook and go to story/components-buttongroup--playground
  2. Inspect the spacing between buttons

Contributor checklist:

  • All new and existing CI checks pass
  • Tests prove that the feature works and covers both happy and unhappy paths
  • Any drop in coverage, breaking changes or regressions have been documented above
  • New visual snapshots have been generated / updated for any UI changes
  • All developer debugging and non-functional logging has been removed
  • Related issues have been referenced in the PR description

Reviewer checklist:

  • Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
  • Check that tests prove the feature works and covers both happy and unhappy paths
  • Check that there aren't other open Pull Requests for the same update/change

Screenshots:

Please try to provide before and after screenshots or videos

Before After

image

image

Copy link

changeset-bot bot commented Dec 11, 2023

🦋 Changeset detected

Latest commit: 8098900

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@primer/react-brand Patch
@primer/brand-primitives Patch
@primer/brand-e2e Patch
@primer/brand-fonts Patch
@primer/brand-config Patch
@primer/brand-storybook Patch

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

@MadMango MadMango marked this pull request as ready for review December 11, 2023 17:26
@rezrah rezrah added the brand label Dec 18, 2023
@rezrah
Copy link
Collaborator

rezrah commented Jan 29, 2024

@MadMango thanks for opening this PR, and sorry for the delay in getting back to you. The Primer team have done some further investigation into the original request, and determined that this won't be something that we fix in the library

The original issue relates to the gap between the button label and adjacent arrow, not the gap between grouped buttons. This PR addresses the latter. We've also discovered that a 4px gap is already present, but optical alignment is leading to some indirection. You can find these details captured in the original issue.

Closing this for now, but thanks again for choosing to contribute to Primer Brand ✨

@rezrah rezrah closed this Jan 29, 2024
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.

🐛 [BUG] - Button Gap on Medium Size should be 4px (0.25rem) instead of 12px.
2 participants