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.

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

Change: make browser validations configurable via DSL #2339

Conversation

dalpo
Copy link
Contributor

@dalpo dalpo commented Jun 20, 2015

Add the ability to enable/disable html5 browser validations through:

RailsAdmin.config do |config|
  config.browser_validations = true
end

@dalpo
Copy link
Contributor Author

dalpo commented Jun 20, 2015

I'm not able to reproduce the failure with mongoid...

@dalpo
Copy link
Contributor Author

dalpo commented Jun 23, 2015

Actually, browser validations are generating a lot of javascript errors for has_many/has_one widget because the input fields are hidden.

For instance:

An invalid form control with name='' is not focusable.
An invalid form control with name='recipe[main_variant_attributes][variant_ingredients_attributes][1435052695601][quantity]' is not focusable.
An invalid form control with name='' is not focusable.
An invalid form control with name='recipe[main_variant_attributes][variant_ingredients_attributes][1435052695601][quantity]' is not focusable.

@dalpo
Copy link
Contributor Author

dalpo commented Jun 28, 2015

@mshibuya Let me know what do you think about this PR 😉

@dalpo dalpo changed the title Change: make bowser validations configurable via DSL Change: make browser validations configurable via DSL Jul 1, 2015
@andreazaupa
Copy link
Contributor

+1

1 similar comment
@giacomomacri
Copy link

+1

@@ -254,6 +257,7 @@ def models
# @see RailsAdmin::Config.registry
def reset
@compact_show_view = true
@browser_validations = false
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit concerned about defaulting this to false.
Don't you think that client-side validation is a nice-to-have feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mshibuya If you prefer I can set the default value to true. It's not a problem for me.

My personal opinion is that browser validation is a nice to have feature only for simple application's models.

In my experience there are two use cases that it could be a trouble:

  • When a model has has_many/has_one nested attributes to be validated the browser-validation is generating a lot of js errors as commented above.
  • When a model has conditional validations.

Let me know what do you prefer as default value so I can do a commit and rebase the PR 😉

@dalpo dalpo force-pushed the feature/configurable-html5-validations branch from 0362e9f to b7d8c64 Compare July 15, 2015 17:51
@dalpo
Copy link
Contributor Author

dalpo commented Jul 15, 2015

@mshibuya I changed the default value to true

@dalpo
Copy link
Contributor Author

dalpo commented Aug 5, 2015

@mshibuya @sferik any news about this PR?

@dalpo dalpo force-pushed the feature/configurable-html5-validations branch from 163105d to 962ee4a Compare August 5, 2015 07:53
@mshibuya
Copy link
Member

mshibuya commented Aug 5, 2015

Squashed and cherry-picked with slight modification(html_required? is unnecessary, because setting novalidate to form is enough). Thanks!

@mshibuya mshibuya closed this Aug 5, 2015
@dalpo
Copy link
Contributor Author

dalpo commented Aug 5, 2015

❤️

Thanks @mshibuya!!

@fmh
Copy link
Contributor

fmh commented Aug 6, 2015

@dalpo

in my case , config.browser_validations = false has no effect, and browser validation still works for me. cannot see novalidate attribute in field

capture d ecran 2015-08-06 a 19 38 24

@dalpo
Copy link
Contributor Author

dalpo commented Aug 6, 2015

@fmh Thanks for reporting!

I fixed it here: #2373

@fmh
Copy link
Contributor

fmh commented Aug 6, 2015

now, it works, thx 👍

@vincentwoo
Copy link
Contributor

@dalpo as you said, I am getting a bunch of the following js errors on rails_admin 0.7:

An invalid form control with name='' is not focusable.
An invalid form control with name='recipe[main_variant_attributes][variant_ingredients_attributes][1435052695601][quantity]' is not focusable.

This did not happen on 0.6.8. Disabling all browser validation feels like overkill. Did we figure out what was wrong in this case?

@dalpo
Copy link
Contributor Author

dalpo commented Aug 26, 2015

@vincentwoo I didn't investigate too much about it.

Btw this was the old PR that introduced the required attribute:
https://github.com/sferik/rails_admin/pull/2170/files

As you can see that PR didn't introduce the required attribute for all input fields, but only for those that are using the _form_field.html.haml and _form_text.html.haml partials.

However with the latest changes, now the required attribute is present for every input field.
May be this could cause your errors.

P.S.
Sorry for my bad English..

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

Successfully merging this pull request may close these issues.

6 participants