-
Notifications
You must be signed in to change notification settings - Fork 351
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
add required option on form_group_builder #488
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR. I think this is a good suggestion. Please add tests and documentation and we'll merge it.
I'd like to merge a PR (#486) to apply some Rubocop style rules to the entire code base of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates to this PR. It's well done, as always. Forgive me for being a nit-picker. I have one comment I'd like you to address, and then we'll merge. See the comment for details.
README.md
Outdated
@@ -219,6 +219,12 @@ In cases where this behavior is undesirable, use the `skip_required` option: | |||
<%= f.password_field :password, label: "New Password", skip_required: true %> | |||
``` | |||
|
|||
In cases where the label `required` class should be added but the associated model has no presence validation, use the `required` option: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a line that says the required: false
does nothing, or, change the code so that required: false
is equivalent to skip_required: true
. If you change the code, don't forget to add a test case for required: false
. If you just change the documentation, I don't think you need to add a test case.
I don't have an opinion about which is the right solution. Since you have the use case, I'm happy to leave it to your judgement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch !
To simplify the API I would do the following :
- deprecate skip_required
- add a warning (like rails is doing)
- raise an error if skip_required and the new required options are used at the same time
- allow 3 values on the new required option:
:infered
: default behavior (required class is added if validation is present)false
: required class NOT added, whatever the model validationtrue
: required class added, whatever the model validation
Or I'll add a line in the readme to say that required: false
does nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the enthusiasm. If you want to modify your PR for what you listed under "To simplify the API...", it sounds good to me.
The only suggestion I have is that we make it a warning rather than an error if both skip_required
and required
are used, and use the skip_required
option. This would prevent breaking sites where someone in the past:
- Used
required
without looking in the docs. - When it didn't work, saw that the option was
skip_required
. - Added
skip_required
but forgot to remove therequired
option.
I know that's a bit extreme, but I think this case warrants a warning rather than raising an exception.
One small detail: "inferred" has two "r". (And no need to apologize. English spelling sucks. I had to look it up myself to be sure.) Or you could just use :default
instead of inferred
. I think some other options use :default
.
Let me look at the Rubocop rules to try to fix the Travis build. I don't think there's anything wrong with your code. We'll probably merge this PR as long as it's only the Rubocop check that's failing. |
lib/bootstrap_form/form_builder.rb
Outdated
@@ -463,8 +464,8 @@ def generate_label(id, name, options, custom_label_col, group_layout) | |||
classes << "mr-sm-2" | |||
end | |||
|
|||
unless options.delete(:skip_required) | |||
classes << "required" if required_attribute?(object, name) | |||
if options.delete(:required) || (!options.delete(:skip_required) && required_attribute?(object, name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take back my earlier comment. Could you please change these lines to the modifier form (if
at the end of the line). That may fix all the RuboCop offences (even if I wish we didn't have such long lines).
Well done! Apologies for my sloppy attempts to fix some of the RuboCop offences. I thought I'd fix them myself rather than nagging you. Your work was excellent. Thanks for all your effort on this PR. |
Add required option on form_group_builder :
This option forces required class to be added, even if the there is no presence validation on the model.
This is usefull when the object model is not an active record model.
I will add tests and doc if you are interested