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.

Sign up for GitHub

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

Jump to bottom

change wrapper element assignment #79

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

change wrapper element assignment #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
87e9fec
AddedTestsForBootstrap4HorizontalForms
MichalRemis Apr 14, 2020
62a6d81
change wrapper element assignment so wrapper_error_class is assigned …
MichalRemis Apr 13, 2020
aa3954e
Added tests for :horizontal_form wrapper and made it pass
MichalRemis Apr 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions dist/simple-form.bootstrap4.esm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,24 @@ ClientSideValidations.formBuilders['SimpleForm::FormBuilder'] = {
wrappers: {
"default": {
add: function add(element, settings, message) {
var wrapperElement = element.parent();
var parentElement = element.parent();
var wrapperElement = element.closest(settings.wrapper_tag + '.' + settings.wrapper_class.replace(/ /g, '.'));
var errorElement = wrapperElement.find(settings.error_tag + '.invalid-feedback');

if (!errorElement.length) {
errorElement = $('<' + settings.error_tag + '>', {
"class": 'invalid-feedback',
text: message
});
wrapperElement.append(errorElement);
parentElement.append(errorElement);
}

wrapperElement.addClass(settings.wrapper_error_class);
element.addClass('is-invalid');
errorElement.text(message);
},
remove: function remove(element, settings) {
var wrapperElement = element.parent();
var wrapperElement = element.closest(settings.wrapper_tag + '.' + settings.wrapper_class.replace(/ /g, '.'));
var errorElement = wrapperElement.find(settings.error_tag + '.invalid-feedback');
wrapperElement.removeClass(settings.wrapper_error_class);
element.removeClass('is-invalid');
Expand Down
7 changes: 4 additions & 3 deletions dist/simple-form.bootstrap4.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,23 +26,24 @@
wrappers: {
"default": {
add: function add(element, settings, message) {
var wrapperElement = element.parent();
var parentElement = element.parent();
var wrapperElement = element.closest(settings.wrapper_tag + '.' + settings.wrapper_class.replace(/ /g, '.'));
var errorElement = wrapperElement.find(settings.error_tag + '.invalid-feedback');

if (!errorElement.length) {
errorElement = $('<' + settings.error_tag + '>', {
"class": 'invalid-feedback',
text: message
});
wrapperElement.append(errorElement);
parentElement.append(errorElement);
}

wrapperElement.addClass(settings.wrapper_error_class);
element.addClass('is-invalid');
errorElement.text(message);
},
remove: function remove(element, settings) {
var wrapperElement = element.parent();
var wrapperElement = element.closest(settings.wrapper_tag + '.' + settings.wrapper_class.replace(/ /g, '.'));
var errorElement = wrapperElement.find(settings.error_tag + '.invalid-feedback');
wrapperElement.removeClass(settings.wrapper_error_class);
element.removeClass('is-invalid');
Expand Down
8 changes: 5 additions & 3 deletions src/main.bootstrap4.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,14 @@ ClientSideValidations.formBuilders['SimpleForm::FormBuilder'] = {
wrappers: {
default: {
add (element, settings, message) {
const wrapperElement = element.parent()
const parentElement = element.parent()
const wrapperElement = element.closest(settings.wrapper_tag + '.' + settings.wrapper_class.replace(/ /g, '.'))

let errorElement = wrapperElement.find(settings.error_tag + '.invalid-feedback')
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

@MichalRemis MichalRemis Apr 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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:
    ...  
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

@MichalRemis MichalRemis Apr 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.


if (!errorElement.length) {
errorElement = $('<' + settings.error_tag + '>', { class: 'invalid-feedback', text: message })
wrapperElement.append(errorElement)
parentElement.append(errorElement)
}

wrapperElement.addClass(settings.wrapper_error_class)
Expand All @@ -29,7 +31,7 @@ ClientSideValidations.formBuilders['SimpleForm::FormBuilder'] = {
},

remove (element, settings) {
const wrapperElement = element.parent()
const wrapperElement = element.closest(settings.wrapper_tag + '.' + settings.wrapper_class.replace(/ /g, '.'))
const errorElement = wrapperElement.find(settings.error_tag + '.invalid-feedback')

wrapperElement.removeClass(settings.wrapper_error_class)
Expand Down
125 changes: 125 additions & 0 deletions test/javascript/public/test/form_builders/validateHorizontalSimpleFormBootstrap4.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
QUnit.module('Validate Horizontal wrapper SimpleForm Bootstrap 4', {
before: function () {
currentFormBuilder = window.ClientSideValidations.formBuilders['SimpleForm::FormBuilder']
window.ClientSideValidations.formBuilders['SimpleForm::FormBuilder'] = BS4_FORM_BUILDER
},

after: function () {
window.ClientSideValidations.formBuilders['SimpleForm::FormBuilder'] = currentFormBuilder
},

beforeEach: function () {
dataCsv = {
html_settings: {
type: 'SimpleForm::FormBuilder',
error_class: 'is-invalid',
error_tag: 'div',
wrapper_error_class: 'form-group-invalid',
wrapper_tag: 'div',
wrapper_class: 'form-group'
},
validators: {
'user[name]': { presence: [{ message: 'must be present' }], format: [{ message: 'is invalid', 'with': { options: 'g', source: '\\d+' } }] },
'user[username]': { presence: [{ message: 'must be present' }] }
}
}

$('#qunit-fixture')
.append(
$('<form>', {
action: '/users',
'data-client-side-validations': JSON.stringify(dataCsv),
method: 'post',
id: 'new_user'
})
.append(
$('<div>', { 'class': 'form-group row' })
.append(
$('<label for="user_name" class="string col-sm-3 col-form-label">Name</label>'))
.append(
$('<div>', { 'class': 'col-sm-9' })
.append(
$('<input />', { 'class': 'form-control', name: 'user[name]', id: 'user_name', type: 'text' }))))
// there isn't horizontal :input_group wrapper in simple_form's bootstrap 4 configuration by default
// but if somebody would do it it would look like this
.append(
$('<div>', { 'class': 'form-group row' })
.append(
$('<label for="user_username" class="string col-sm-3 col-form-label">Username</label>'))
.append(
$('<div>', { 'class': 'col-sm-9' })
.append(
$('<div>', { 'class': 'input-group' })
.append(
$('<div>', { 'class': 'input-group-prepend' })
.append(
$('<span>', { 'class': 'input-group-text', text: '@' })))
.append(
$('<input />', { 'class': 'form-control', name: 'user[username]', id: 'user_username', type: 'text' }))))))

$('form#new_user').validate()
}
})

var wrappers = ['horizontal_form' ]

for (var i = 0; i < wrappers.length; i++) {
var wrapper = wrappers[i]

QUnit.test(wrapper + ' - Validate error attaching and detaching', function (assert) {
var form = $('form#new_user')
var input = form.find('input#user_name')
var label = $('label[for="user_name"]')
form[0].ClientSideValidations.settings.html_settings.wrapper = wrapper

input.trigger('focusout')
assert.ok(input.closest('.form-group').hasClass('form-group-invalid'))
assert.ok(label.parent().hasClass('form-group-invalid'))
assert.ok(input.parent().find('div.invalid-feedback:contains("must be present")')[0])

input.val('abc')
input.trigger('change')
input.trigger('focusout')
assert.ok(input.closest('.form-group').hasClass('form-group-invalid'))
assert.ok(input.parent().find('div.invalid-feedback:contains("is invalid")')[0])
assert.ok(input.hasClass('is-invalid'))

input.val('123')
input.trigger('change')
input.trigger('focusout')
assert.notOk(input.closest('.form-group').parent().hasClass('form-group-invalid'))
assert.notOk(input.parent().parent().find('div.invalid-feedback:contains("is invalid")')[0])
assert.notOk(input.hasClass('is-invalid'))
})

QUnit.test(wrapper + ' - Validate pre-existing error blocks are re-used', function (assert) {
var form = $('form#new_user'); var input = form.find('input#user_name')
var label = $('label[for="user_name"]')
form[0].ClientSideValidations.settings.html_settings.wrapper = wrapper

input.parent().append($('<div class="invalid-feedback">Error from Server</span>'))
assert.ok(input.parent().find('div.invalid-feedback:contains("Error from Server")')[0])
input.val('abc')
input.trigger('change')
input.trigger('focusout')
assert.ok(input.closest('.form-group').hasClass('form-group-invalid'))
assert.ok(label.parent().hasClass('form-group-invalid'))
assert.ok(input.parent().find('div.invalid-feedback:contains("is invalid")').length === 1)
assert.ok(form.find('div.invalid-feedback').length === 1)
})

QUnit.test(wrapper + ' - Validate input-group', function (assert) {
var form = $('form#new_user'); var input = form.find('input#user_username')
form[0].ClientSideValidations.settings.html_settings.wrapper = wrapper

input.trigger('change')
input.trigger('focusout')
assert.ok(input.closest('.input-group-prepend').find('div.invalid-feedback').length === 0)
assert.ok(input.closest('.input-group').find('div.invalid-feedback').length === 1)

input.val('abc')
input.trigger('change')
input.trigger('focusout')
assert.ok(input.closest('.input-group').find('div.invalid-feedback').length === 0)
})
}
7 changes: 4 additions & 3 deletions vendor/assets/javascripts/rails.validations.simple_form.bootstrap4.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,23 +26,24 @@
wrappers: {
"default": {
add: function add(element, settings, message) {
var wrapperElement = element.parent();
var parentElement = element.parent();
var wrapperElement = element.closest(settings.wrapper_tag + '.' + settings.wrapper_class.replace(/ /g, '.'));
var errorElement = wrapperElement.find(settings.error_tag + '.invalid-feedback');

if (!errorElement.length) {
errorElement = $('<' + settings.error_tag + '>', {
"class": 'invalid-feedback',
text: message
});
wrapperElement.append(errorElement);
parentElement.append(errorElement);
}

wrapperElement.addClass(settings.wrapper_error_class);
element.addClass('is-invalid');
errorElement.text(message);
},
remove: function remove(element, settings) {
var wrapperElement = element.parent();
var wrapperElement = element.closest(settings.wrapper_tag + '.' + settings.wrapper_class.replace(/ /g, '.'));
var errorElement = wrapperElement.find(settings.error_tag + '.invalid-feedback');
wrapperElement.removeClass(settings.wrapper_error_class);
element.removeClass('is-invalid');
Expand Down