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

Issue #24173 Custom checkboxes radios correct color #24401

Merged
merged 6 commits into from
Oct 22, 2017

Conversation

sabrown84
Copy link

@sabrown84 sabrown84 commented Oct 16, 2017

Hi I took a look at this issue and I made the suggested changes to:

Custom Checkboxes/Radios Do Not Display Correct Color on :focus For Validation

Is this what you meant? Is there something else that needs to be done for this issue?

I welcome and look forward to your feedback.

Fixes #24173

Sharrell Porter added 2 commits October 16, 2017 16:57
Merge branch 'custom-checkboxes-radios-correct-color' of github.com:sabrown84/bootstrap into custom-checkboxes-radios-correct-color
Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

Hey there @sabrown84, this is super close! There are two fixes we need you to make though before we can merge. The referenced issue is for the outline on focus of those fields, so we need to move this new rule to a different selector. Second is moving from hex values to some Sass variables.

I took a stab at outlining those changes in a couple comments—let me know if you have any questions about it!

@@ -79,6 +79,7 @@
&.is-#{$state} {
~ .custom-control-indicator {
background-color: rgba($color, .25);
box-shadow: 0 0 0 1px #fff, 0 0 0 3px #dc3545;
Copy link
Member

Choose a reason for hiding this comment

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

Because this ruleset is in a Sass mixin, and we're passing parameters for the color, you'll want to tweak this line just a bit. Rather than hardcode the hex colors, use this:

box-shadow: 0 0 0 1px $body-bg, 0 0 0 3px $color;

This way, if you customize the $body-bg variable, it'll get picked up automatically here. And when we use this mixin to generate the valid and invalid styles, that color will be correct, too.

Copy link
Member

Choose a reason for hiding this comment

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

After you make that change, we can move this to the focus state.

.was-validated &:#{$state},
&.is-#{$state} {
  ~ .custom-control-indicator {
    background-color: rgba($color, .25);
  }
  &:focus {
    ~ .custom-control-indicator {
      box-shadow: 0 0 0 1px $body-bg, 0 0 0 3px $color;
    }
  }
}

Copy link
Author

Choose a reason for hiding this comment

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

Thank you @mdo I will make these changes send another commit. This is so helpful!

@mdo mdo merged commit 1083e49 into twbs:v4-dev Oct 22, 2017
@mdo
Copy link
Member

mdo commented Oct 22, 2017

Pulled this down, made a few more commits to address some opacities, and also fixed the file input focus state.

@mdo mdo mentioned this pull request Oct 22, 2017
@sabrown84
Copy link
Author

Thank you @mdo. I learned a few things from this one.

@tmorehouse
Copy link
Contributor

tmorehouse commented Oct 25, 2017

Note for custom-file inputs, the focus styling will not work in Firefox, due to how Firefox treats what is "focused" in the native file input. Firefox focuses the button inside the file input and not the file input as a whole, so the sibling selector will not work in the Firefox case.

See https://stackoverflow.com/a/20104275 and http://jsfiddle.net/Yh8re/2/ (use Firefox to test) and http://jsfiddle.net/Yh8re/23/

Firefox treats the file button and the filename label as two nested elements. Clicking the filename will cause a focus sibling selector to work, but clicking the browse button will not.
This is especially noticeable for firefox keyboard users.

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.

4 participants