Skip to content

Conversation

@dancormier
Copy link
Contributor

@dancormier dancormier commented Jul 25, 2022

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

Note see also: #1135

New features

.s-btn-group using input[type="radio"]

This PR includes changes to .s-btn-group to support implementation with input[type="radio"]. Normally, I'd separate these into unique PRs, but this and .s-btn are pretty tightly coupled, so I've included the change here.

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

TODO

  • Rename PPCP to match format (--_btn should become --_bt)
  • Fix hover styling on selected button within .s-btn-group using input[type="radio"]

@netlify
Copy link

netlify bot commented Jul 25, 2022

Deploy Preview for stacks ready!

Name Link
🔨 Latest commit 77c425c
🔍 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.

@dancormier dancormier marked this pull request as ready for review September 27, 2022 22:07
@dancormier dancormier marked this pull request as draft November 4, 2022 21:08
@dancormier dancormier marked this pull request as ready for review November 9, 2022 18:25
Copy link
Contributor

@b-kelly b-kelly left a comment

Choose a reason for hiding this comment

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

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?
  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)
  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

"description": "Base button group style."
},
{
"class": ".s-btn-group--container",
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is a breaking change. How should we handle this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I originally intended this to be a non-breaking change, but a deprecation. In 77c425c, I've targeted the form element and once again made this a deprecation. Basically with the PR as it is now, the changes should be backwards compatible.

image

Note
Image for demo purposes only. Don't worry. I didn't change the docs to this permanently.

Should we reintroduce .s-btn-group--container? I know targeting a HTML tag feels a little icky but it doesn't feel totally wrong here. That said, I'm fine with targeting that class and punting on any of this and FWIW it's pretty low stakes since I only found it used in one place in Core.


PS: .s-btn-group--container previously just set any nested button border radius to 0. That means if the button was at the start or end of the button group, it would get squared off. I've addressed this in 77c425c.

@dancormier
Copy link
Contributor Author

dancormier commented Nov 11, 2022

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 e946105

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 dancormier requested a review from b-kelly November 11, 2022 19:25
Copy link
Contributor

@b-kelly b-kelly left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the updates

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants