Allow to configure default form attributes #562

sharshenov · 2020-04-14T21:57:14Z

This PR adds ability to configure bootstrap_form default form options

# config/initializers/bootstrap_form.rb
BootstrapForm.configure do |c|
  c.default_form_attributes = { something: "custom" }
end

Current default value is {role: "form"} which is not semantically correct as described in #560. In order to resolve original issue, default_form_attributes should be set to {}

BootstrapForm.configure do |c|
  c.default_form_attributes = {}
end

Closes #561
Closes #560

lcreid

Thanks for this PR. Nice work!

So if I have an application using bootstrap_form version 4.4.0 (the current released version), and something in my app that depends on forms having role="form", will the app break if I just bundle update to version 4.5.0 (if it includes this PR)?

If so, we need to think of the most practical way to maintain version 4 behaviour, and then make no role="form" the default for version 5 (the Bootstrap 5 version).

Thanks again for the great work. Don't be discouraged by my comments. We try really hard to make sure we don't break existing applications, except at major version upgrades (which are Bootstrap major version upgrades).

lcreid · 2020-04-15T14:50:35Z

README.md

@@ -150,6 +150,18 @@ in `form_with`.

 `form_with` has some important differences compared to `form_for` and `form_tag`, and these differences apply to `bootstrap_form_with`. A good summary of the differences can be found at: https://m.patrikonrails.com/rails-5-1s-form-with-vs-old-form-helpers-3a5f72a8c78a, or in the [Rails documentation](api.rubyonrails.org).

+
+## Configuration


I'd like to give a little more information to the reader around configuration. I suggest something like a first paragraph that says:

bootstrap_form can be used out-of-the-box without any configuration. However, bootstrap_form does have an optional configuration file at config/initializers/bootstrap_form.rb for setting options that affect all generated forms in an application.

The current configuration options are:

default_form_attributes bootstrap_form versions 4.4.0 and older added a role="form" attribute to all forms. The W3C validator will raise a warning on forms with a role="form" attribute.

The actual example will depend on the path we choose for maintaining compatibility for version 4, and enabling the new default for version 5 (which will be the Bootstrap v5 version). See my comments on the pull request.

sharshenov · 2020-04-15T18:33:44Z

@lcreid

So if I have an application using bootstrap_form version 4.4.0 (the current released version), and something in my app that depends on forms having role="form", will the app break if I just bundle update to version 4.5.0 (if it includes this PR)?

No. Default configuration still makes role="form" attribute be rendered. So this change is not breaking. It allows to set default_form_attributes with custom hash (like {})

If so, we need to think of the most practical way to maintain version 4 behaviour, and then make no role="form" the default for version 5 (the Bootstrap 5 version).

I think, the best way is to declare a Github milestone and create an issue to drop role: "form" from BootstrapForm::Configuration::DEFAULT and then link the issue to the milestone.
I could also add a post_install_message to announce this breaking change in upcoming major version. WDTY? Should I?

I'd like to give a little more information to the reader around configuration

Updated in ce7bc3d (I'll squash to first commit if it is approved)

lcreid

Nice work! Thanks for clearing up my confusion. I was staring at the file that had the initialization, but it didn't sink in what I was seeing. I just have one more question/suggestion with part A and B. Take a look and let me know what you think.

I really appreciate this PR. It's really nice and clean. Thanks!

lcreid · 2020-04-16T02:01:34Z

lib/bootstrap_form/form_builder.rb

      options[:html] ||= {}
-      options[:html][:role] ||= "form"
+      options[:html].reverse_merge!(BootstrapForm.config.default_form_attributes)


It would be nice to make this work reasonably if the user sets default_form_attributes = nil. Extra bonus marks if we nicely handle if the user sets default_form_attributes to something other than a hash. Throwing an error on start-up might be better than waiting until the app actually tries to render a form.

sharshenov · 2020-04-19T15:25:45Z

@lcreid Now configuration raises error if unsupported value is given for default_form_attributes. I've also added post install message.
I'd like to squash all commits if you consider PR good enough.

P.S. Travis fails because of unrelated problem with rubocop configuration

lcreid

Nice work. Thanks for the PR!

lcreid · 2020-04-20T02:55:40Z

bootstrap_form.gemspec

@@ -15,6 +15,8 @@ Gem::Specification.new do |s|
                  "easy to create beautiful-looking forms using Bootstrap 4"
  s.license     = "MIT"

+  s.post_install_message = "Default form attribute role=\"form\" will be dropped in 5.0.0"


lcreid · 2020-04-20T02:57:03Z

lib/bootstrap_form/configuration.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+module BootstrapForm


sharshenov mentioned this pull request Apr 14, 2020

Implement Configuration Options #561

Closed

Allow to configure default form attributes

8d3c786

sharshenov force-pushed the configurable-default-form-attributes branch from 9cd6c52 to 8d3c786 Compare April 15, 2020 09:42

lcreid reviewed Apr 15, 2020

View reviewed changes

[SQUASH LATER] Update Readme

ce7bc3d

lcreid assigned lcreid and sharshenov and unassigned lcreid Apr 16, 2020

lcreid reviewed Apr 16, 2020

View reviewed changes

[SQUASH LATER] Validate default_form_attributes config argument

5d2d6a7

sharshenov force-pushed the configurable-default-form-attributes branch from 21fa0f9 to 5d2d6a7 Compare April 19, 2020 14:52

lcreid approved these changes Apr 20, 2020

View reviewed changes

lcreid merged commit 9ae52df into bootstrap-ruby:master Apr 20, 2020

sharshenov deleted the configurable-default-form-attributes branch May 5, 2020 00:12

lcreid mentioned this pull request Jun 21, 2020

Remove form attribute role="form" #569

Closed

4 tasks

lcreid mentioned this pull request Mar 28, 2021

Errors on helper uses weird class names #579

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to configure default form attributes #562

Allow to configure default form attributes #562

sharshenov commented Apr 14, 2020 •

edited

Loading

lcreid left a comment

lcreid Apr 15, 2020

sharshenov commented Apr 15, 2020

lcreid left a comment

lcreid Apr 16, 2020

sharshenov commented Apr 19, 2020

lcreid left a comment

lcreid Apr 20, 2020

lcreid Apr 20, 2020

		@@ -150,6 +150,18 @@ in `form_with`.

		`form_with` has some important differences compared to `form_for` and `form_tag`, and these differences apply to `bootstrap_form_with`. A good summary of the differences can be found at: https://m.patrikonrails.com/rails-5-1s-form-with-vs-old-form-helpers-3a5f72a8c78a, or in the [Rails documentation](api.rubyonrails.org).


		## Configuration

		@@ -0,0 +1,23 @@
		# frozen_string_literal: true

		module BootstrapForm

Allow to configure default form attributes #562

Allow to configure default form attributes #562

Conversation

sharshenov commented Apr 14, 2020 • edited Loading

lcreid left a comment

Choose a reason for hiding this comment

lcreid Apr 15, 2020

Choose a reason for hiding this comment

sharshenov commented Apr 15, 2020

lcreid left a comment

Choose a reason for hiding this comment

lcreid Apr 16, 2020

Choose a reason for hiding this comment

sharshenov commented Apr 19, 2020

lcreid left a comment

Choose a reason for hiding this comment

lcreid Apr 20, 2020

Choose a reason for hiding this comment

lcreid Apr 20, 2020

Choose a reason for hiding this comment

sharshenov commented Apr 14, 2020 •

edited

Loading