StackExchange / Stacks

Stack Overflowโ€™s Design System
https://stackoverflow.design
MIT License
602 stars 89 forks source link

chore(button-group): update design #1630

Closed dancormier closed 4 months ago

dancormier commented 5 months ago

STACKS-554

This PR contains updates to the button group component.

Updates to markup

Button styles

In order to leverage existing .s-btn styles as much as possible, this component does not significantly modify button borders. Consumers should remove any existing .s-btn__outlined from buttons within .s-btn-group.

Preventing layout shift w/ the .s-btn--text element

Consumers will need to wrap text in a span.s-btn--text[data-text="[button text]"] element to prevent layout shift. Without it, the component will mostly render as expected by may shrink/grow a few pixels horizontally.

Remaining concerns @CGuindon

  1. This design is wider than the previous design, which could cause layout shifts/wrapping in for consumers
    • I found them to be 5-15% wider depending on the contents of the button group
  2. ~Do we need to consider hover/active states specifically for these buttons in the context of button group?~
    • ~Specifically, the hover state is very subtle for the .s-btn__muted with the border color removed~

Original notes for posterity (Feb 2, 2024)

@CGuindon This PR has a preliminary version of the updated button group component. There are a quirks that are probably worth discussing.

  1. This design is wider than the previous design, which could cause layout shifts/wrapping in for consumers
    • I found them to be 5-15% wider depending on the contents of the button group
  2. I haven't found an acceptable way to compensate for badges contained within buttons in a button group and I'm concerned there might not be a reasonable way to handle this
    • This is because the badge is a standalone element with it's own padding/margin that can't be reproduced in the :before pseudo-element approach that I've taken
    • The best option I see at this stage is to deal with the slight bit of layout shift (which I'm not into)
  3. Do we need to consider hover/active states specifically for these buttons in the context of button group?
    • Specifically, the hover state is very subtle for the .s-btn__muted with the border color removed
  4. Consumers will need to remove s-btn__outlined from any existing buttons within button groups.
  5. The approach I took will require consumers add a data-text=[button text] attribute to the button
    • It won't be difficult, but possibly a little tedious
    • If this is omitted, the button group is likely to shift around slightly when selecting a button due to the bold text

Note Note for Dan - The docs will need to be updated to detail data-attr usage

netlify[bot] commented 5 months ago

Deploy Preview for stacks ready!

Name Link
Latest commit 537defbbf58658c8759cde7e6678b01536eec41f
Latest deploy log https://app.netlify.com/sites/stacks/deploys/65de61c62b2aee0008b2e340
Deploy Preview https://deploy-preview-1630--stacks.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

CGuindon commented 5 months ago

@dancormier โ€” if I understood the concern correctly, no, not as part of this change. Hover state: we're matching muted button which is also very subtle but feels like it's own issue we can worry about at a later time Hover + Active โ€” looks like we don't do any visual changes for that on Buttons

dancormier commented 5 months ago

Have we considered alternative approaches like text-shadow?

I don't view text-shadow as the equivalent to bolded text. Bolded fonts are custom designs of a given font whereas text shadow is a bit of a blunt instrument. We'd lose crisp edges and wind up with kinda a blobby look to the selected element.

Using text shadow

image

Using bold

image

My desired approach (that's not yet supported) FWIW

Eventually, browsers will support content: contents which will just reproduce the contents of a given element as the pseudoelement. I don't know when this will be available in browsers though.

dancormier commented 5 months ago

Not sure if this is a Stacks thing or a Radio thing โ€” in the last button group example, the hover state is different on the active button. We don't have that now... so is that on purpose? Our other active states don't have a hover, should this new version maintain that? Screen.Recording.2024-02-06.at.4.15.25.PM.mov

Ah, this is a bug. We have hover styles applied when a button does not have the is-selected class on it. This works as expected with the standard button groups, but doesn't on radio button groups since the is-selected class is never applied. Nice catch @CGuindon. It'll be a tricky issue to solve but I'll poke around and see if I can figure it out in this PR.

dancormier commented 4 months ago

Ok, I figured out the hover issue on radio-based button groups (the solution is pretty simple but was surprising tricky to get to). This should be ready for review now ๐ŸŽ‰

dancormier commented 4 months ago

Looks good to me @dancormier. Nice work. Once this PR is merged - what's the plan?

  • Cut Stacks Classic 2.4.0

Stacks Classic 2.3.0 I believe, but yep

  • Perform the upgrade in Core (ourselves given that this can be considered an a11y fix and we have this new mandate now?)

I'm comfortable with us performing this upgrade

Yep, though this is by no means a rush. Currently the ButtonGroup Razor component is not referenced in Core.