Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[v4] Improve form control validation #29228

Closed
wants to merge 2 commits into from
Closed

Conversation

browner12
Copy link
Contributor

Problem

Currently server side validation requires an .invalid-feedback element to be a sibling of a .form-control element. This works fine most of the time. However, sometimes we have elements to don't fit into standard form control inputs. For example, I have multi-column checkbox area that allows for overflow scroll in the y direction.

<div class="col-12 col-sm-10">
    <div class="form-control is-invalid rounded p-2">
        <div class="row" style="height: 200px; overflow-y: scroll; ">
            @foreach($domains as $domain)
                <div class="col-12 col-md-6 col-lg-4">
                    <div class="form-check form-check-inline">
                        <input name="domains[]" type="checkbox" id="domains.{{ $domain->id }}" class="form-check-input" value="{{ $domain->id }}" />
                        <label class="form-check-label" for="domains.{{ $domain->id }}">{{ $domain->domain }}</label>
                    </div>
                </div>
            @endforeach
        </div>
    </div>
    <div class="invalid-feedback">Something went wrong.</div>
</div>

However, wrapping this element in a .form-control causes a lot of unwanted styling, that can often break the layout of it.

Proposed Solution

This change allows us to apply any class that starts with .form-control to the element, and still trigger showing .valid-feedback and .invalid-feedback elements. Now we can wrap any non-standard form controls in the class of our choosing, and still get validation.

<div class="form-control-no-styling is-invalid rounded p-2"></div>
<div class="invalid-feedback">Something went wrong.</div>

switch from an exact match of `.form-control`, to matching any class that begins with `.form-control`.
@browner12 browner12 requested a review from a team as a code owner August 8, 2019 17:03
@MartijnCuppens
Copy link
Member

MartijnCuppens commented Aug 8, 2019

Hi @browner12,

Could you provide a Codepen demo of the problem/solution?

@XhmikosR XhmikosR changed the base branch from master to actions August 8, 2019 18:02
@XhmikosR XhmikosR changed the base branch from actions to master August 8, 2019 18:04
@browner12
Copy link
Contributor Author

@martinbean
Copy link
Contributor

@browner12 I don’t think you should be changing the selector.

I use exactly the same pattern in my Laravel apps. Instead, consider creating a custom CSS class. You can override properties with it:

.form-control-scrollable {
  height: auto;
  max-height: 200px;
  overflow-y: auto;
}

You can then combine the classes:

<div class="form-control form-control-scrollable">
  <!-- Checkboxes -->
</div>
<div class="invalid-feedback"><!-- Message --></div>

@browner12
Copy link
Contributor Author

I disagree. If I don't want the default .form-control styling, that shouldn't create a hurdle for me to utilize the awesome validation classes that BS has.

@martinbean
Copy link
Contributor

@browner12

<div class="invalid-feedback d-block"></div>

No need to change Bootstrap’s selectors.

@browner12
Copy link
Contributor Author

Yes, we can do that, but now we have to add conditional logic for the .d-block class to our feedback. Doable, but you lose out consistency now because some inputs work one way, while others are handled another.

Another benefit we get with my change is the .form-control-whatever class also gets the colored bordered upon validation.

Do you have a specific reason or concern to not add this?

@martinbean
Copy link
Contributor

martinbean commented Aug 9, 2019

Do you have a specific reason or concern to not add this?

@browner12 My concern is, you’re wanting Bootstrap to change to accommodate your particular use case, which can be solved with a modifier class.

Example: https://codepen.io/martinbean/pen/WVKYVL

@browner12
Copy link
Contributor Author

Obviously no proof, but I would guess other people have had this issue as well. Wait, scratch that, you've had this issue, but you've come up with your own workaround.

Your solution uses custom classes, but with the rise of utility classes, I've come to the point where I use few to no custom classes in my entire site. Your solution is great if you want to use a custom class, but I'd like to make it possible for people to do it without one.

@martinbean
Copy link
Contributor

martinbean commented Aug 9, 2019

Obviously no proof, but I would guess other people have had this issue as well. Wait, scratch that, you've had this issue, but you've come up with your own workaround.

It’s not an “issue” in the sense that it’s a bug with Bootstrap and needs “fixing”.

The “issue” is re-purposing Bootstrap’s .form-control class for something that isn’t a form control in itself. Therefore, I created a modifier class. A utility class. With four lines of CSS. That I can use across my application.

It isn’t in Bootstrap’s remit to cater for everyone’s application out of the box so that you never have to write a single line of CSS in your application again.

We clearly have a vastly different opinion on the subject, so I’ll leave this now and let someone actually involved in the Bootstrap project decide whether this is merged or not.

@mdo
Copy link
Member

mdo commented Aug 15, 2019

Reading through this, I don't think this is a large enough problem to warrant changing our selectors for the validation. I do believe we need a reworking of our validation setup (see #28995 for example), but they will still be explicitly tied to the form elements and their associated classes.

I do think we need something else for checks/radios because while the individual checks/radios currently support form validation styles, radios likely would benefit more from a revied, single validation for the entire group.

@browner12
Copy link
Contributor Author

yah, the checkbox/radio styling and validation styling is definitely a little quirky. just ran into it today.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants