-
-
Notifications
You must be signed in to change notification settings - Fork 835
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
Make checkbox switch component background stand out in modals #2443
Make checkbox switch component background stand out in modals #2443
Conversation
- Disabled checkbox switch now has body (white by default) background within modals - Still turns green when on - Still has control background outside of modals Fixes #2119
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.
Thank you!
less/common/Checkbox.less
Outdated
.Modal-content & { | ||
background: @body-bg; | ||
} | ||
|
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.
For consistency, this should be added to the Modal.less file.It should also be applied to .Modal-body
since that's the only part with the @control-bg
background.
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.
Thanks @SychO9, agree on both points. Have made the modifications - please have a look. Note that due to the way LESS appends and prepends parent selectors the added styles in Modal.less have to be a bit more verbose than essentially a one-liner in Checkbox.less.
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.
Thank you, I think we might be able to use a one liner it we used the off
class, can you please test if that's the case ?
less/common/Modal.less
Outdated
@@ -105,6 +105,14 @@ | |||
text-align: left; | |||
} | |||
} | |||
|
|||
.Checkbox--switch .Checkbox-display { |
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.
I think if we use .off.Checkbox--switch .Checkbox-display
here instead we could probably avoid having to declaring the block under it 😃
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.
Yep, just gave it a go - it works, thanks for the suggestion!
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.
Looks good to me!
I'm not a huge fan of the white on the switch since the actual switching part is white as well. What about |
@KyrneDev, here's what it looks like with |
Oh I like that better! Good idea |
Updated the PR with the new background - commit message looks weird because background variable names were in backticks and git ate them :). |
Since the checkbox switch doesn't currently have a disabled state CSS style, that looks better indeed.
keeps happening to me lol |
The last thing I'd like to see before approving: how does this look in dark mode? |
@askvortsov1, the dark mode looks like this (see screenshot below). Great with the |
Thank you! |
Fixes #2119
Changes proposed in this pull request:
As discussed in the linked issue, make checkbox switch background white in modals as otherwise the component is barely discernible. Once I looked at the actual styles used, I decided that a better option would be to use the
body-bg
variable instead of hardcoding white.Reviewers should focus on:
UI changes.
Screenshot
Confirmed