-
Notifications
You must be signed in to change notification settings - Fork 843
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
Compressed switch styling #2327
Conversation
@cchaos Feel free to directly push any stylistic changes you see in this PR. Compressed switches are basically half size versions of the existing ones with a smaller font. Due to the removal of the icons, I added a slightly darker border to them which I think they need to help some contrast issues with the originals. I'm hopeful the way I managed the vars and overwrites keeps it maintainable, since the compressed versions simply pull directly from the non-compressed ones, but if you see any improvements there go for it. Set this to merge to master since it's a non-breaking change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snide thanks for adding me as a reviewer! 👍
In my opinion, the switch height seems very small. When we're comparing with other compressed form components it seems not part of the "family".
I checked the other compressed components and the Radio and Checkboxes have 16px (height). My suggestion would be to have the same height.
All the rest seems good! 🎉
@miukimiu yep, good point. My usage is within the datagrid, which requires even more compact needs. Dunno if I need to go into making a third, micro option, but that's down a rabbit hole. I think even when upping the height to 16px, we'd want to increase the length a bit as well. I'll play around and see if I can find a happy medium. |
Talked with @miukimiu about this. I'm gonna take her suggestion and make this 16px, then use an additive class for datagrid that makes it even smaller (similar to what this is now) for that usage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and +1 the conversation thus far. I, too, favor the larger height suggested by @miukimiu .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Just had a couple questions
Summary
Adds compressed styling to switches. We already had them at the prop level, they just didn't do anything.
Checklist
Added or updated jest testsChecked for breaking changes and labeled appropriatelyChecked for accessibility including keyboard-only and screenreader modes