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

Compressed switch styling #2327

Merged
merged 5 commits into from
Sep 11, 2019
Merged

Compressed switch styling #2327

merged 5 commits into from
Sep 11, 2019

Conversation

snide
Copy link
Contributor

@snide snide commented Sep 11, 2019

Summary

Adds compressed styling to switches. We already had them at the prop level, they just didn't do anything.

Checklist

  • Checked in dark mode
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@snide snide requested a review from cchaos September 11, 2019 03:50
@snide snide mentioned this pull request Sep 11, 2019
13 tasks
@snide
Copy link
Contributor Author

snide commented Sep 11, 2019

@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.

Copy link
Contributor

@miukimiu miukimiu left a 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.

Compressed-switch

All the rest seems good! 🎉

@snide
Copy link
Contributor Author

snide commented Sep 11, 2019

@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.

@snide
Copy link
Contributor Author

snide commented Sep 11, 2019

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.

Copy link
Contributor

@ryankeairns ryankeairns left a 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 .

Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

image
I too think this new size works better! Code LGTM.

Copy link
Contributor

@cchaos cchaos left a 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

src/components/form/switch/_switch.scss Outdated Show resolved Hide resolved
src/components/form/switch/_switch.scss Outdated Show resolved Hide resolved
@snide snide merged commit 520d6dc into elastic:master Sep 11, 2019
@snide snide deleted the switch/compressed branch September 11, 2019 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants