change wrapper element assignment

[MichalRemis]

Original wrapper assignment to .parent() of the element doesn't work for all simple_form's wrappers, as wrapper isn't always parent of the input (e.g. :horizontal_form wrapper). Searching by new way should work better for simple_form's wrappers. I tested it with :vertical_form, :horizontal_form, :select (radios,checkboxes and also date doesn't work anyway but according to generated HTML this method should work once CSV supports these input types). BTW I took this line from #65 (comment)

[coveralls]

Coverage remained the same at 100.0% when pulling aa3954e on MichalRemis:patch-1 into f43f395 on DavyJonesLocker:master.

[tagliala]

Calling find on a String, this will fail. The comment used element.closest that is missing here

[MichalRemis]

Yes, sorry may fault, I made a mistake while copy&pasting. Downloaded the repo locally now and made sure that test are passing in next commit. Also I think e.g. validateSimpleFormBootstrap4.js test for horizontal_form SimpleForm's bootstrap wrapper isn't absolutely correct, because current SimpleForm's bootstrap configuration would make HTML for field with error like this:

<div class="form-group row integer required form-group-invalid">
    <label class="col-sm-3 col-form-label" for="xxx">
      Label
    </label>
    <div class="col-sm-9">
      <input class="form-control is-invalid" name="xxx" id="xxx">
      <div class="invalid-feedback">Error feedback...</div>
      <small class="form-text text-muted">Input hint..</small>
    </div>
</div>

So the form-group-invalid class isn't on parent of the input but on the wrapper and that's what I am trying to address with my PR.

[tagliala]

Thanks!

Feel free to fix the test

Anyway, In this case, I think that the error should be added to the parent element. Maybe we can add a check on the form's css class and provide two different approaches

[MichalRemis]

Ok I will fix the test but then the current element.parent() method of adding wrapper_error_class would be failing with :horizontal_form wrapper. I just thought new element.closest('.form-group') method is more universal for current simple_form's bootstrap4 configuration working with most of it's wrappers.

I don't think there's wrapper's class on form's css, because wrapper is a simple_form's thing, not bootstrap's. If different approaches are needed (and I think it will be for more complex wrappers), CSV has a nice way to do this by implementing wrapper's specific add/remove methods in ClientSideValidations.formBuilders['SimpleForm::FormBuilder'] e.g.:

wrappers: {
  horizontal_form: {
    add (element, settings, message) {
      ...
    },
    remove...
      ...
  default:
    ...  
}

[tagliala]

Ok, I would go for the closest form group, but it will not work for any use case.

Probably I went for element.parent instead of the closest wrapper that I've suggested because at the time I wrote this solution it was the choice that worked with most of the combinations

[MichalRemis]

Ok I can see what you mean. The error message (.invalid-feedback) should be added to parent element, in that case element.parent() is right. But when adding wrapper_error_class, wrapper tag isn't always the parent. Maybe we should split this.. add error to parentElement and add wrapper_error_class to wrapperElement.

[MichalRemis]

I will fix/add tests for horizontal_wrapper, vertical_wrappers and inline_wrapper according to current simple_form default bootstrap config and we may test it against those, what do you think?

[tagliala]

Ok I can see what you mean. The error message (.invalid-feedback) should be added to parent element, in that case element.parent() is right. But when adding wrapper_error_class, wrapper tag isn't always the parent. Maybe we should split this.. add error to parentElement and add wrapper_error_class to wrapperElement.

Yes, it is mostly correct

wrapper_errror_class should not be needed at all by Bootstrap 4 itself, we could remove that

I can see that all examples provided here: https://getbootstrap.com/docs/4.4/components/forms/ (search for invalid-feedback) are attached to the parent element (actually the sibling of the input element, but it should not be the case of input-group-append).

I will fix/add tests for horizontal_wrapper, vertical_wrappers and inline_wrapper according to current simple_form default bootstrap config and we may test it against those, what do you think?

If you have a comprehensive page with a lot of form elements generated by the latest default bootstrap+simple form, please share the html here (or in a gist), so I can also take a look

[MichalRemis]

You can find latest simple_form examples at http://simple-form-bootstrap.plataformatec.com.br/ (submit forms for invalid markup). You are right most of wrappers add invalid-feedback as sibling of the input, except for Input Group Form (but yes, bootstrap4 docs has it as sibling so maybe it works both ways. This also cause double invalid-feedback when (not) re-using existing server error block.) and special date/radio/etc.. inputs which CSV doesn't support anyway.

Anyway purpose of my PR was to fix wrapper_errror_class assignment which ATM doesn't work for Horizontal Form and Input Group Form examples. You are right wrapper_errror_class is not needed by bootstrap but it's SimpleForm's thing and people may expect it there and could use it e.g. for styling of label. Sure it's possible to customize JS formBuilder but it would be nice if it works by default.

I committed JS tests for :horizontal_form wrapper and made them pass with a change which I believe also solves double invalid-feedback for input-group fields.

[tagliala]

Hi!

Thanks for being part of the CSV Community and thanks for this PR.

I got the issue, this is a possible breaking change, but specs should not pass.

In fact, there is an error and new javascripts have not been compiled: https://travis-ci.org/github/DavyJonesLocker/client_side_validations-simple_form/jobs/674596631#L397

I should fix that, meanwhileyou should be able to run rake test:js on your development machine

[MichalRemis]

Firstly thank you for great gem. I would like to use it but it seems that it doesn't cope well with more complex SimpleForm's forms. I would like to fix it to make this nice gem better. Some problem's I found:

• CSV doesn't take into account field's specific wrapper. In simple_form you may specify simple_form's wrapper for every field with wrapper: argument. Current CSV is only using wrapper set for the whole form.

• CSV doesn't work for checkboxes/radios/simple_form's :date field (multi select) and simple_form's fields defined with f.associations call, (probably also f.input_field, f.collections_check_boxes).

I saw discussion about the radios issue and lot of people request this and I see you request help for it. I would like to help with all this, do you maybe have some ideas how to implement it? If not, it's ok I think I can solve it somehow.

[tagliala]

@MichalRemis Please rebase on master, I've pushed a fix for tests that didn't fail properly