Simple_form: form-group-valid class added on Edit

[Petercopter]

When loading a record to edit, I didn't expect the validity styles to be applied. The form should be in a 'default' state, without styles success/error styles. This is with the Bootstrap style initializer.

EDIT: I see the same behavior in two different applications with 4.0.1 installed. Both simple Rails apps.

Environment

• Ruby 2.5.1
• Rails 5.2
• Simple Form 4.0.1

Current behavior

When loading a page to edit a record, form-group-valid is already applied to the form input groups (see the green outline on the inputs)

Expected behavior

The forms inputs shouldn't have the form-group-valid classes applied until it comes back from being submitted.

[Petercopter]

Actually, I'm pretty sure this is working as intended, but it still seems.... overkill-ish to me. The default state of an input is valid, I guess I just wasn't expecting to see styling on the fields until the form came back from being submitted.

I ended up removing , valid_class: 'is-valid' from my simple_form_bootstrap initializer file, which will probably achieve what I'm expecting. I'd be curious about what other people think.

[LoveToCodeRuby]

@Petercopter I also believe this functionality is intended, but just doesn't seem right if the form wasn't submitted prior.

[jrochkind]

I agree this is kind of unexpected and probably not desirable UX for many apps, in the simplest case.

It's in lots of places in the generated bootstrap config, so it's kind of tricky to take the generated bootstrap config and then apply config changes on top to customize. Not sure how to handle this in my app.

Maybe it should be expected that one will just customize the generated bootstrap-compat config, that's why it's generated?

[feliperenan]

This behavior was not intended when we introduced this feature. At least I didn't think about the Edit case 🙈.

I will try to figure out how to solve it. By now, if you don't want this behavior, just remove the option valid_class from the wrapper.

I'm also open for suggestions 😄

[jrochkind]

Thanks for the update!

To be clear about what I myself am seeing... it does not appear to be the form-group-valid class that is making all the text inputs be outlined in green.

Rather, it's an is-valid class on the individual inputs, no?

<input class="form-control is-valid string required" type="text" value="foo" name="work[title]" id="work_title">

If I remove the is-valid class in chrome inspector -- no longer green.

But removing the form-group-valid on the containing div -- still (undesirably) green.

Can you say more what you mean by "just remove the option valid_class from the wrapper" -- you're talking about the generated simple_form_bootstrap.rb? valid_class: 'form-group-valid' appears I think 24 times in there, so each of those? Remove the arg entirely, or empty string instead?

Except what is probably needed instead is removing the valid_class: 'is-valid', which appears about the same number of times.

Is the generated simple_form_bootstrap.rb file generally intended to be customized locally? I wasn't sure if you generally intended the user to edit it to meet local needs, or to instead leave it alone so if there are updates they can be easily re-generated on top without losing local changes. The file does begin with a comment # Please do not make direct changes to this file! -- but not sure if that's supposed to be talking to me the end-developer who has generated the file (with that comment!) into my app, or to someone looking at the template in the original source repo... or what.

[jrochkind]

Also interested in hearing the use-case you intended for it. I can't think of any place in a 'standard' Rails app I'd want it -- I think I might only want the "valid" class if I had some special JS that validated the field as I entered it or something? I'm not sure why I'd ever want it on a server-rendered page, as the simple form config will effect.

I wonder if it should just be removed from the config, as clever as it is. But maybe I'm missing the intended use case where it would be welcome when it shows up?

[feliperenan]

Can you say more what you mean by "just remove the option valid_class from the wrapper" -- you're talking about the generated simple_form_bootstrap.rb? valid_class: 'form-group-valid' appears I think 24 times in there, so each of those? Remove the arg entirely, or empty string instead?

Sorry, I was not clear. You need to remove the valid_class: 'is-valid from the input in the wrapper:

https://github.com/plataformatec/simple_form/blob/67d8df4768f7b5759502df6f0bae8160b6556d0c/lib/generators/simple_form/templates/config/initializers/simple_form_bootstrap.rb#L61

Regarding motivations about this feature, you can see in the PR:

https://github.com/plataformatec/simple_form/pull/1559
https://github.com/plataformatec/simple_form/pull/1553#issuecomment-374530817

[jrochkind]

Cool, thanks. Personally I think there is simply no need for valid-class in a server-side-rendered forms situation, and it's fine just to leave it out entirely -- rather than figure out some way to get it only activated when re-rendering forms on an error, which I gather was the intent, but seems like it might require some kind of complex implementation.

But clearly others disagree! :)

[jrochkind]

How is this config related?

   config.input_field_valid_class = 'is-valid'

In my generated config/initializers/simple_form_bootstrap.rb

Does the is-valid actually need to be repeated everywhere, if it's there already?

[feliperenan]

Now, reading those PRs I remember that we brought this caveat instead having one more dependency on Simple Form. Maybe we should disable this is-valid by default since the most users (I think) will not use that?

If that makes sense, feel free to open a PR :).

[jrochkind]

@feliperenan are you imagining a PR to this repo, of the bootstrap template generating code? Or does there need to be a PR to rafaelfranca/simple_form-bootstrap first, because they are supposed to match? Or are you imagining a different sort of PR to a different area of code? I'm confused about the 'right' way to do this.

[feliperenan]

I meant a PR for this repo removing the is-valid from the bootstrap generator, that way this class would not be included by default unless the developer want to enable this. We may need to add docs explaining how it works and how active this.

[jrochkind]

@feliperenan Thanks, I'll see if I can submit that PR.

Another thing I've been confused about is that the generated file has a line config.input_field_valid_class = 'is-valid', but then _also_ repeats that in every one of it's numerous wrappers, eg bb.use :input, class: 'form-check-input', error_class: 'is-invalid', valid_class: 'is-valid'.

I don't understand what the top-level config.input_field_error_class and config.input_field_valid_class are for, if they don't actually effect those wrappers, and those class directives need to be repeated in each one. It would be a lot easier for an individual developer-user to decide they wanted to turn on or off the valid-class behavior if they could do that by editing _one_ line, the top-level config.input_field_error_class, instead of having to do it for every defined wrapper (_plus_ that top-level line, which I'm not sure if it actually affects anything?).

[feliperenan]

f.input_field and f.input are different things at Simple Form. The first one generates just the input tag and the second one generates all markup defined in your wrapper. So they require different configurations.

https://github.com/plataformatec/simple_form/blob/bd1fd283cab765ca46b6a49ffac7029c2b4fe530/lib/simple_form/form_builder.rb#L164

https://github.com/plataformatec/simple_form/blob/bd1fd283cab765ca46b6a49ffac7029c2b4fe530/lib/simple_form/form_builder.rb#L117

[feliperenan]

Closing in favor https://github.com/plataformatec/simple_form/pull/1651#issuecomment-505898114