-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[v4] [core] fix(Switch): fix text color and indicator border #5058
Conversation
@@ -48,13 +48,13 @@ $button-box-shadow-overlay-active: | |||
inset 0 1px 1px rgba($black, 0.1) !default; | |||
|
|||
$dark-button-box-shadow: | |||
inset 0 0 0 1px rgba(17, 20, 24, 0.8) !default; | |||
inset 0 0 0 $button-border-width rgba($black, 0.8) !default; |
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.
using variables instead of hardcoded values
@@ -300,6 +300,10 @@ $control-indicator-spacing: $pt-grid-size !default; | |||
|
|||
$switch-indicator-text-font-size: 0.7em; | |||
|
|||
$switch-text-color: $pt-text-color !default; |
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.
Explicitly setting text colors for switch's inner text (for light/dark and enabled/disabled)
@@ -321,19 +329,23 @@ $control-indicator-spacing: $pt-grid-size !default; | |||
$switch-indicator-background-color: $white !default; | |||
$switch-indicator-background-color-disabled: rgba($white, 0.8) !default; | |||
$dark-switch-indicator-background-color: $dark-gray5 !default; | |||
$dark-switch-indicator-box-shadow: 0 0 0 $button-border-width rgba($black, 0.8) !default; |
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.
Explicitly set dark switch indicator's box shadow to not be inset, to be consistent with other indicator box shadows.
Remove inset border for dark checked switch indicatorPreviews: documentation | landing | table | modern colors demo |
} | ||
|
||
input:checked ~ .#{$ns}-control-indicator::before { | ||
// inset shadow so it forms a dark line instead of blurring into the blue |
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.
since the box shadow is different now, we don't get this blurring effect anymore without the inset
@@ -275,104 +275,7 @@ table.#{$ns}-html-table { | |||
} | |||
} | |||
|
|||
// Contrast for switches |
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.
Removed these overrides, now built in the backgrounds into switch variables
|
||
&::before { | ||
background: $disabled-indicator-color; | ||
box-shadow: none; |
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.
no borders for disabled switch indicator
@adidahiya this PR should be ready for review now! |
Fix switch indicator background colors and bordersPreviews: documentation | landing | table | modern colors demo |
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.
this is kind of an an explosion of Sass variables... probably unnecessary complexity which makes the code harder to work with... but since that's kind of the existing code pattern for these styles, we can leave it as-is and try to refactor in a separate PR 🙃
code changes look fine, and visuals lgtm... I'll wait for @aycai for final design approval
yeah, I was pretty sad adding all these new variables, but given that the switch styles have diverged slightly from the button styles, it didn't make sense to use the existing button variables. I'll think of ways to improve this in a follow up - I think low hanging fruit is to remove variables that are only used once (which I'd imagine there are a few of)
|
@johnly13 Shoot, didn't show switch with text in the mocks - light mode + enabled + disabled state text should be 60% opacity to match other states, but rest looks good! |
Done - updated screenshots above as well! |
60% opacity for light mode disabled textPreviews: documentation | landing | table | modern colors demo |
Fixes #5046 (comment)
Checklist
Changes proposed in this pull request:
box-shadow
instead ofinset
box-shadow
for dark switch indicatorsReviewers should focus on:
Screenshot
(note: I think the indicator looks off center vertically in some of these screenshots. I think this is an artifact of zooming in + screenshotting, rather than how they actually are styled)