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

Form check markup #23444

Closed
wants to merge 16 commits into
base: v4-dev
from

Conversation

Projects
None yet
5 participants
@mdo
Member

mdo commented Aug 15, 2017

This fixes #23426, but might constitute as a breaking change.

What changes

Validating an input within our .form-check component is straightforward—it'll handle :valid and :invalid with ease. However, styling the label's text based on the input's state is another thing. Let's look at what we have now:

<div class="form-check">
  <label class="form-check-label">
    <input type="checkbox" class="form-check-input">
    Check me out
  </label>
</div>

With the above HTML, you cannot style Check me out in CSS based on .form-check-input's state. To make that happen, we need a sibling element to style, something like:

<div class="form-check">
  <label class="form-check-label">
    <input type="checkbox" class="form-check-input">
    <span class="form-check-description">Check me out</span>
  </label>
</div>

That's what this PR changes all our checkboxes and radios to. This matches custom forms more and enables CSS-only validation of checkboxes/radios.

Breaking or non-breaking?

So, why the question of maybe breaks backward compatibility? It's not strictly required. Right now, there's no need to include it for anything but validation. However, it could help improve styling of disabled inputs as well. Here's the current CSS for disabled .form-checks, which requires a parent class:

bootstrap/scss/_forms.scss

Lines 218 to 228 in 5b8738a

.form-check {
position: relative;
display: block;
margin-bottom: $form-check-margin-bottom;
&.disabled {
.form-check-label {
color: $text-muted;
}
}
}

With this change, we could do:

.form-check {
  position: relative;
  display: block;
  margin-bottom: $form-check-margin-bottom;
}

.form-check-input:disabled {
  + .form-check-description {
    color: $text-muted;
  }
}

Thoughts?

@hiddewie

This comment has been minimized.

Show comment
Hide comment
@hiddewie

hiddewie Aug 15, 2017

I agree this is a fix for #23426, however it does not address the issue where a whole set of radio buttons or checkboxes is valid/invalid, with an invalid description for the whole set.

Considering just server side validation, a value for a field (which might be a single value <-> radio buttons, or multiple values <-> checkboxes) is invalid. This means that not the <input>s themselves should get the .is-invalid class, but maybe the <fieldset> containing the inputs. However the <fieldset> would not have .form-control which does not render invalid form elements as invalid, or the invalid description.

See the image for a more detailed view of what I mean. It was made with BS alpha 4 I believe.

hiddewie commented Aug 15, 2017

I agree this is a fix for #23426, however it does not address the issue where a whole set of radio buttons or checkboxes is valid/invalid, with an invalid description for the whole set.

Considering just server side validation, a value for a field (which might be a single value <-> radio buttons, or multiple values <-> checkboxes) is invalid. This means that not the <input>s themselves should get the .is-invalid class, but maybe the <fieldset> containing the inputs. However the <fieldset> would not have .form-control which does not render invalid form elements as invalid, or the invalid description.

See the image for a more detailed view of what I mean. It was made with BS alpha 4 I believe.

@acidghost

This comment has been minimized.

Show comment
Hide comment
@acidghost

acidghost Sep 23, 2017

What is the status of this PR? If it is likely that the .form-check-description change will happen, I'll monkey patch my code for the time being.

I see the problem @hiddewie shows, wouldn't adding something like the following work?

fieldset.is-#{state} {
    .form-check-description {
        color: $color;
    }
}

acidghost commented Sep 23, 2017

What is the status of this PR? If it is likely that the .form-check-description change will happen, I'll monkey patch my code for the time being.

I see the problem @hiddewie shows, wouldn't adding something like the following work?

fieldset.is-#{state} {
    .form-check-description {
        color: $color;
    }
}
@mdo

This comment has been minimized.

Show comment
Hide comment
@mdo

mdo Oct 2, 2017

Member

Status is this needs reviewing, testing, and ideally merging :).

Member

mdo commented Oct 2, 2017

Status is this needs reviewing, testing, and ideally merging :).

@mdo

This comment has been minimized.

Show comment
Hide comment
@mdo

mdo Nov 25, 2017

Member

Need to look at #23503 as well.

Member

mdo commented Nov 25, 2017

Need to look at #23503 as well.

@mdo mdo referenced this pull request Dec 21, 2017

Merged

Form check markup v2 #25050

@mdo

This comment has been minimized.

Show comment
Hide comment
@mdo

mdo Dec 21, 2017

Member

Superseded by #25050.

Member

mdo commented Dec 21, 2017

Superseded by #25050.

@mdo mdo closed this Dec 21, 2017

@mdo mdo removed this from Needs review in v4 Beta 3 Dec 21, 2017

@mdo mdo deleted the form-check-markup branch Dec 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment