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

Make checkbox switch component background stand out in modals #2443

Merged
merged 4 commits into from
Nov 10, 2020
Merged

Make checkbox switch component background stand out in modals #2443

merged 4 commits into from
Nov 10, 2020

Conversation

nina-py
Copy link
Contributor

@nina-py nina-py commented Nov 7, 2020

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

  • Disabled checkbox switch now has body background (white by default) within modals

switch-off-on-modal

  • Still turns green when on

switch-on-on-modal

  • Still has control background outside of modals when off

switch-elsewhere

Confirmed

  • Frontend changes: tested on a local Flarum installation.

- 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
Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

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

Thank you!

Comment on lines 34 to 37
.Modal-content & {
background: @body-bg;
}

Copy link
Member

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.

Copy link
Contributor Author

@nina-py nina-py Nov 8, 2020

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.

Copy link
Member

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 ?

@@ -105,6 +105,14 @@
text-align: left;
}
}

.Checkbox--switch .Checkbox-display {
Copy link
Member

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 😃

Copy link
Contributor Author

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!

Copy link
Member

@SychO9 SychO9 left a 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!

@KyrneDev
Copy link
Contributor

KyrneDev commented Nov 9, 2020

I'm not a huge fan of the white on the switch since the actual switching part is white as well. What about @muted-more-color for the background?

@nina-py
Copy link
Contributor Author

nina-py commented Nov 9, 2020

@KyrneDev, here's what it looks like with @muted-more-color rather than white:

muted-more-colour

@askvortsov1
Copy link
Sponsor Member

Oh I like that better! Good idea

@nina-py
Copy link
Contributor Author

nina-py commented Nov 9, 2020

Updated the PR with the new background - commit message looks weird because background variable names were in backticks and git ate them :).

@SychO9
Copy link
Member

SychO9 commented Nov 9, 2020

Since the checkbox switch doesn't currently have a disabled state CSS style, that looks better indeed.

commit message looks weird because background variable names were in backticks and git ate them :).

keeps happening to me lol

@askvortsov1
Copy link
Sponsor Member

The last thing I'd like to see before approving: how does this look in dark mode?

@nina-py
Copy link
Contributor Author

nina-py commented Nov 10, 2020

@askvortsov1, the dark mode looks like this (see screenshot below). Great with the muted-more-color, it would have been essentially black-on-black with the body background color.

dark-mode-switch

@askvortsov1 askvortsov1 merged commit 67741c7 into flarum:master Nov 10, 2020
@askvortsov1
Copy link
Sponsor Member

Thank you!

@askvortsov1 askvortsov1 modified the milestone: 0.1.0-beta.15 Nov 10, 2020
@nina-py nina-py deleted the 2119-switch-component-background branch November 10, 2020 01:59
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.

Switch component background merges with Modal background
4 participants