Stacks icon indicating copy to clipboard operation
Stacks copied to clipboard

refactor(button): style with pseudo-private custom properties

Open dancormier opened this issue 1 year ago • 1 comments

This PR refactors the CSS for .s-btn to use pseudo-private custom properties.

Benefits

Reduced fragility, increased maintainability

By defining a subset of rules that are modified by custom properties on a per-variant-basis, there's lower likelihood of the cascade biting us. With a handful of exceptions, .s-btn rules are set at the component level and only custom properties are modified within variants and modifiers without the need to rewrite boilerplate CSS.

Bugs fixes

I noticed a few issues that I resolved by preventing CSS rule conflicts.

High contrast primary button active state

In develop, the high contrast primary button style doesn't change on :active. This PR resolves that.

High contrast borders

Some buttons variants had inconsistent borders in high contrast (Primary style selected state, for example). This PR resolves those issues.

20220725-061300

Bundle size 🤷‍♂️

This PR drops ~2KB from stacks.min.css (though I suspect this could be a wash when gzipped. TBD)

dancormier avatar Jul 25 '22 22:07 dancormier

Deploy Preview for stacks ready!

Name Link
Latest commit 77c425c9c1f68cb8d06015f89a58ba180b22ee6d
Latest deploy log https://app.netlify.com/sites/stacks/deploys/636e9e8edca3850009c90996
Deploy Preview https://deploy-preview-1038--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 settings.

netlify[bot] avatar Jul 25 '22 22:07 netlify[bot]

Excellent work! Visually, this PR doesn't appear to introduce any regressions (tested all combos between light/dark, standard/custom theme, highcontrast on/off). Code-wise, the organization of this file has been greatly improved (dropping ~200 lines of duplicated selectors/properties in the process).

🎉

I do have a few easily addressed concerns:

1. Looks like the removal of the documented `s-btn-group--container` class is a breaking change. I'd prefer to avoid this if at all possible. Any chance we can put a shim in here with a deprecation note in the changelog?

Yeah, I thought that it could be deprecated and any relevant CSS could be removed, but I guess I didn't compensate for this properly (bottom row includes form.s-btn-group--container):

image

I'll have to reconsider removing this class before merging this PR.

2. Radio based buttons don't have focus rings. Since this syntax is new to this PR, we could do one of the following:
   
   * just add the checked selectors to the focused selectors, much like you did for `.is-selected`
   * change the markup to have the input _inside_ of the label, then drop the special selector casing for `:focus-within` (in addition to `:focus`)

Fixed in https://github.com/StackExchange/Stacks/pull/1038/commits/e946105efad6abb90de5368385556c9ffc415a01

3. I also spotted an existing bug you can fix while you're in here: Primary buttons don't change their BG color when hovering over them in _dark mode_

Ah, nice catch! I'll fix that. Update: fixed in e519ab4

dancormier avatar Nov 11 '22 18:11 dancormier