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

fix(check, radio): increased size, added accent color #4637

Closed
wants to merge 1 commit into from

Conversation

mcoker
Copy link
Contributor

@mcoker mcoker commented Jan 22, 2022

fixes #4322


Looking at what I think are the best ways to do this, we can:

  1. Use radios/checks as they are, just increase their size and apply accent-color set to our primary color. That's the change in this PR, and I created a codepen demo here - https://codepen.io/mcoker/pen/JjrzgEv
  • Pros:
    • Retains the browser default shape and interactive behavior.
    • Inputs are larger (a11y++) and IMO look better.
    • CSS is super simple. Only a few lines per component. I feel a lot more comfortable making this a global change for all checkboxes/radios - not just our checkbox/radio component. Meaning if someone includes a random input that doesn't use our component, it will look like the component.
  • Cons:
    • accent-color isn't supported in Safari (yet), though the default isn't far off from our primary blue so I don't think it's a big deal. Seems like a good opportunity for a progressive enhancement.
    • The right side of radio inputs seems to be cut off in Safari. Should be fixable though, didn't spend much time on it.
  1. Use pseudo elements to recreate/draw the inputs. The basic idea here is to hide the default input, and attach a style to it that lets us create our own "element" that represents the input. Here's a codepen with that - https://codepen.io/mcoker/pen/rNGREgj.
  • Pros:
    • Gives us and users significantly more flexibility over how to draw/animate/customize the inputs.
    • It's probably easier to align these elements with labels/text across browsers since they don't include the default input CSS per browser which is kind of a pain to work with with to get the element to align properly. Though I'd want to work on the alignment between the approaches a little more to get a better idea.
  • Cons:
    • We lose the default browser shape/interactive behavior. This would probably make it a breaking visual change since these inputs look different across browsers.
    • CSS is quite a bit more complex. Definitely a code breaking change.
    • There may be a11y implications with this since it's a bit more intrusive on the styling side by hiding the default input. But that may not be a factor given we're able to hide the default input in a browser friendly way.

In both cases, the increased width does push the text beside the input over by a couple of pixels. The horizontal layout shift could potentially cause an input + label (or an inline row of multiple input + labels) to break to a new line when it didn't previously. Likely an edge case, and seems acceptable to me.

IMO the first solution is the preferred approach, at least for the short term. Seems like something we could do now. The other approach, given proper time to work on may be better in the long term since it allows for way more customization and would make the inputs consistent across browsers.

@patternfly-build
Copy link

patternfly-build commented Jan 22, 2022

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

@mcoker In general, I like the first option, too. I like how it makes the checkbox and radio a bit larger and uses a consistent PatternFly color. The second option feels more like something we might consider for our next breaking change/visual refresh release. I'm a little concerned that a larger checkbox/radio might have adverse effects of spacing in product and cause wrapping where it did not occur before. I also tested this in Safari to see what it looked like. We would probably want to try and fix the radio clipping, but in general, I doubt that we have many users running Safari (although I have no data on that).

Right now, looks like this PR is only applying to the Check and Radio components, if that right? If we make this change, would also want to consider updates to other components using checkboxes for consistency. Assuming that's still a TBD since they continue to have old styling right now.

@mceledonia @mmenestr will be interested in your thoughts.

@mcoker
Copy link
Contributor Author

mcoker commented Jan 24, 2022

@mcarrano another option could just be to update the colors. That's non-invasive and super subtle, though again it wouldn't work in Safari until it has support for the feature.

Right now, looks like this PR is only applying to the Check and Radio components, if that right?

Yep. We could make either 1) the first option or 2) just the color update a global change so that all checkbox/radio elements have that style (not just our component styles). This could also be an opt-in.

One edge case that I can see might be the mix and match of other/third-party applications used in a PF application via iframe or something like that - if the third-party app doesn't use PF styles and has browser default buttons/checkboxes, the styles may not match with the main app. Though I don't know how much of a concern that is because the third-party app's styles probably wouldn't match PF's anyways. Just a thought.

@mmenestr
Copy link

mmenestr commented Feb 2, 2022

I think Option 1 seems like the best option! Since the Safari blue is so close to our blue, I wouldn't change that if there's a risk of mix and match when our blue isn't supported.

@mcarrano
Copy link
Member

@mcoker I've been thinking about this more. Seems like there is definitely interest in this update, although I'm leery of making a global change as it is potentially visual breaking. Should we either consider creating a new variant (custom checkbox?) or some form of opt-in? Let me know what you think and we can discuss.

@mmenestr
Copy link

mmenestr commented Feb 21, 2022

@mcarrano I feel like even if it is technically a breaking change, I don't see it affecting layouts too too much. Worst cause scenario, if there are layouts where there were 4 checkboxes in one line, and now only 3 fit in one line, the 4th one would just wrap below it no? I don't think creating a new variant would make much sense

@mcoker
Copy link
Contributor Author

mcoker commented Apr 22, 2022

deferring for breaking change release

@mcoker mcoker closed this Apr 22, 2022
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.

check box styles
4 participants