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

Move errors inside input-group and form-group. #422

Closed
wants to merge 18 commits into from
Closed

Move errors inside input-group and form-group. #422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
42d2c5a
Test case for #417.
lcreid Feb 3, 2018
d7ef709
Undoing mess created on wrong branch.
lcreid Feb 3, 2018
187aa69
Remove test that accidentally leaked in from another branch.
lcreid Feb 4, 2018
b25ab77
Working but needs cleanup #417.
lcreid Feb 4, 2018
463486e
Partially cleaned up.
lcreid Feb 4, 2018
9e66706
Remove unneeded method and parameters.
lcreid Feb 4, 2018
e83bf64
Test selects error.
lcreid Feb 5, 2018
a2b4586
Test selects error.
lcreid Feb 5, 2018
e48451d
time_zone_select works for new error placement.
lcreid Feb 5, 2018
b2d832a
Add tests for date, time, datetime selects.
lcreid Feb 5, 2018
073a1e3
Add tests for date, time, datetime selects.
lcreid Feb 5, 2018
844e133
date, time, datetime tests pass with old implementation.
lcreid Feb 5, 2018
ebab68a
Merge branch 'new-builder' into 417-input-group
lcreid Feb 5, 2018
0a73344
Date, time, and datetime for new implementation.
lcreid Feb 5, 2018
365dbf2
Tests for file and collection selects.
lcreid Feb 5, 2018
5cc7869
Merge branch 'error-tests' into 417-input-group
lcreid Feb 5, 2018
a15d06d
File and collection selects with new implementation.
lcreid Feb 5, 2018
d12e146
Merge branch 'master' into 417-input-group
lcreid Feb 15, 2018
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
33 changes: 24 additions & 9 deletions lib/bootstrap_form/form_builder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def initialize(object_name, object, template, options)

define_method(with_method_name) do |name, options = {}|
form_group_builder(name, options) do
prepend_and_append_input(options) do
prepend_and_append_input(name, options) do
send(without_method_name, name, options)
end
end
Expand All @@ -53,7 +53,9 @@ def initialize(object_name, object, template, options)

define_method(with_method_name) do |name, options = {}, html_options = {}|
form_group_builder(name, options, html_options) do
content_tag(:div, send(without_method_name, name, options, html_options), class: control_specific_class(method_name))
control_error_help(name, options) do
content_tag(:div, send(without_method_name, name, options, html_options), class: control_specific_class(method_name))
end
end
end

Expand All @@ -63,15 +65,17 @@ def initialize(object_name, object, template, options)
def file_field_with_bootstrap(name, options = {})
options = options.reverse_merge(control_class: 'form-control-file')
form_group_builder(name, options) do
file_field_without_bootstrap(name, options)
control_error_help(name, options) do
file_field_without_bootstrap(name, options)
end
end
end

bootstrap_method_alias :file_field

def select_with_bootstrap(method, choices = nil, options = {}, html_options = {}, &block)
form_group_builder(method, options, html_options) do
prepend_and_append_input(options) do
prepend_and_append_input(method, options) do
select_without_bootstrap(method, choices, options, html_options, &block)
end
end
Expand All @@ -81,23 +85,29 @@ def select_with_bootstrap(method, choices = nil, options = {}, html_options = {}

def collection_select_with_bootstrap(method, collection, value_method, text_method, options = {}, html_options = {})
form_group_builder(method, options, html_options) do
collection_select_without_bootstrap(method, collection, value_method, text_method, options, html_options)
control_error_help(method, options) do
collection_select_without_bootstrap(method, collection, value_method, text_method, options, html_options)
end
end
end

bootstrap_method_alias :collection_select

def grouped_collection_select_with_bootstrap(method, collection, group_method, group_label_method, option_key_method, option_value_method, options = {}, html_options = {})
form_group_builder(method, options, html_options) do
grouped_collection_select_without_bootstrap(method, collection, group_method, group_label_method, option_key_method, option_value_method, options, html_options)
control_error_help(method, options) do
grouped_collection_select_without_bootstrap(method, collection, group_method, group_label_method, option_key_method, option_value_method, options, html_options)
end
end
end

bootstrap_method_alias :grouped_collection_select

def time_zone_select_with_bootstrap(method, priority_zones = nil, options = {}, html_options = {})
form_group_builder(method, options, html_options) do
time_zone_select_without_bootstrap(method, priority_zones, options, html_options)
control_error_help(method, options) do
time_zone_select_without_bootstrap(method, priority_zones, options, html_options)
end
end
end

Expand Down Expand Up @@ -131,6 +141,7 @@ def check_box_with_bootstrap(name, options = {}, checked_value = "1", unchecked_

label_classes = [options[:label_class]]
label_classes << hide_class if options[:hide_label]
error_text = generate_help(name, options.delete(:help)).to_s

if options[:custom]
div_class = ["custom-control", "custom-checkbox"]
Expand All @@ -143,6 +154,7 @@ def check_box_with_bootstrap(name, options = {}, checked_value = "1", unchecked_
# TODO: Notice we don't seem to pass the ID into the custom control.
checkbox_html.concat(label(label_name, label_description, class: label_class))
end
.concat(error_text)
end
else
wrapper_class = "form-check"
Expand All @@ -157,6 +169,7 @@ def check_box_with_bootstrap(name, options = {}, checked_value = "1", unchecked_
label_description,
{ class: label_class }.merge(options[:id].present? ? { for: options[:id] } : {})))
end
.concat(error_text)
end
end
end
Expand All @@ -179,6 +192,7 @@ def radio_button_with_bootstrap(name, value, *args)
disabled_class = " disabled" if options[:disabled]
label_classes = [options[:label_class]]
label_classes << hide_class if options[:hide_label]
error_text = generate_help(name, options.delete(:help)).to_s

if options[:custom]
div_class = ["custom-control", "custom-radio"]
Expand All @@ -191,6 +205,7 @@ def radio_button_with_bootstrap(name, value, *args)
# TODO: Notice we don't seem to pass the ID into the custom control.
radio_html.concat(label(name, options[:label], value: value, class: label_class))
end
.concat(error_text)
end
else
wrapper_class = "form-check"
Expand All @@ -203,6 +218,7 @@ def radio_button_with_bootstrap(name, value, *args)
radio_html
.concat(label(name, options[:label], { value: value, class: label_class }.merge(options[:id].present? ? { for: options[:id] } : {})))
end
.concat(error_text)
end
end
end
Expand Down Expand Up @@ -238,7 +254,6 @@ def form_group(*args, &block)
content_tag(:div, options.except(:id, :label, :help, :icon, :label_col, :control_col, :layout)) do
label = generate_label(options[:id], name, options[:label], options[:label_col], options[:layout]) if options[:label]
control = capture(&block).to_s
control.concat(generate_help(name, options[:help]).to_s)

if get_group_layout(options[:layout]) == :horizontal
control_class = options[:control_col] || control_col
Expand Down Expand Up @@ -351,7 +366,7 @@ def form_group_builder(method, options, html_options = nil)

wrapper_class = css_options.delete(:wrapper_class)
wrapper_options = css_options.delete(:wrapper)
help = options.delete(:help)
help = options[:help]
icon = options.delete(:icon)
label_col = options.delete(:label_col)
control_col = options.delete(:control_col)
Expand Down
28 changes: 23 additions & 5 deletions lib/bootstrap_form/helpers/bootstrap.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,33 @@ def custom_control(*args, &block)
form_group_builder(name, options, &block)
end

def prepend_and_append_input(options, &block)
options = options.extract!(:prepend, :append, :input_group_class)
##
# Add prepend and append, if any, and error if any.
# If anything is added, the whole thing is wrapped in an input-group.
def prepend_and_append_input(name, options, &block)
control_error_help(name,
options,
prepend: options.delete(:prepend),
append: options.delete(:append),
&block)
end

##
# Render a block, and add its error or help.
# Add prepend and append if provided, and wrap if they were, or if
# an input_group_class was provided.
def control_error_help(name, options, prepend: nil, append: nil, &block)
error_text = generate_help(name, options.delete(:help)).to_s
options = options.extract!(:input_group_class)
input_group_class = ["input-group", options[:input_group_class]].compact.join(' ')

input = capture(&block)

input = content_tag(:div, input_group_content(options[:prepend]), class: 'input-group-prepend') + input if options[:prepend]
input << content_tag(:div, input_group_content(options[:append]), class: 'input-group-append') if options[:append]
input = content_tag(:div, input, class: input_group_class) unless options.empty?
input = content_tag(:div, input_group_content(prepend), class: 'input-group-prepend') + input if prepend
input << content_tag(:div, input_group_content(append), class: 'input-group-append') if append
input << error_text
# FIXME: TBC The following isn't right yet. Wrap if there were errors. Maybe???
input = content_tag(:div, input, class: input_group_class) unless options.empty? && prepend.nil? && append.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this FIXME still valid? If so, could you clarify what additional cases need to be handled here?

input
end

Expand Down
2 changes: 1 addition & 1 deletion test/bootstrap_checkbox_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,8 @@ class BootstrapCheckboxTest < ActionView::TestCase
<div class="form-check">
<input class="form-check-input" id="user_misc_1" name="user[misc][]" type="checkbox" value="1" />
<label class="form-check-label" for="user_misc_1"> Foobar</label>
<small class="form-text text-muted">With a help!</small>
</div>
<small class="form-text text-muted">With a help!</small>
</div>
HTML

Expand Down
15 changes: 15 additions & 0 deletions test/bootstrap_fields_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,21 @@ class BootstrapFieldsTest < ActionView::TestCase
assert_equivalent_xml expected, @builder.file_field(:misc)
end

test "file fields are wrapped correctly with error" do
@user.errors.add(:misc, "error for test")
expected = <<-HTML.strip_heredoc
<form accept-charset="UTF-8" action="/users" class="new_user" enctype="multipart/form-data" id="new_user" method="post" role="form">
<input name="utf8" type="hidden" value="&#x2713;"/>
<div class="form-group">
<label for="user_misc">Misc</label>
<input class="form-control-file is-invalid" id="user_misc" name="user[misc]" type="file" />
<div class="invalid-feedback">error for test</div>
</div>
</form>
HTML
assert_equivalent_xml expected, bootstrap_form_for(@user) { |f| f.file_field(:misc) }
end

test "hidden fields are supported" do
expected = %{<input id="user_misc" name="user[misc]" type="hidden" />}
assert_equivalent_xml expected, @builder.hidden_field(:misc)
Expand Down
62 changes: 46 additions & 16 deletions test/bootstrap_form_group_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,32 @@ class BootstrapFormGroupTest < ActionView::TestCase
assert_equivalent_xml expected, @builder.text_field(:email, prepend: '$', append: '.00')
end

test "adding both prepend and append text with validation error" do
@user.email = nil
assert @user.invalid?

expected = <<-HTML.strip_heredoc
<form accept-charset="UTF-8" action="/users" class="new_user" id="new_user" method="post" role="form">
<input name="utf8" type="hidden" value="&#x2713;"/>
<div class="form-group">
<label class="required" for="user_email">Email</label>
<div class="input-group">
<div class="input-group-prepend">
<span class="input-group-text">$</div>
</div>
<input class="form-control is-invalid" id="user_email" name="user[email]" type="text" />
<div class="input-group-append">
<span class="input-group-text">.00</span>
</div>
<div class="invalid-feedback">can't be blank, is too short (minimum is 5 characters)</span>
</div>
</div>
</form>
HTML
# TODO: We should build the @builder properly from `bootstrap_form_for`, so it's easier to test errors.
assert_equivalent_xml expected, bootstrap_form_for(@user) { |f| f.text_field :email, prepend: '$', append: '.00' }
Copy link
Contributor

Choose a reason for hiding this comment

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

I would render @builder.text_field :email, prepend: "foo", append: "bar" instead of doing it though form_helper. Less html that way. All tests should just call helpers directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's how I started, but then it renders the default Rails error markup. @builder is missing the bootstrap_form override of the default Rails error rendering. I was going to enter an issue to change the way we make the builder, so it would render properly without the extra HTML, which I agree is a unnecessary and can be a pain.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can put code used for error suppression (https://github.com/bootstrap-ruby/bootstrap_form/blob/master/lib/bootstrap_form/helper.rb#L45) into setup and teardown for tests that need it. You'll want to extract all tests that deal with error rendering into a separate file though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good suggestion. In the spirit of limiting the scope of each PR, I continued to just tolerate a few extra lines of HTML in each test, but we should revisit the suggestion for easier error testing in another PR.

end

test "help messages for default forms" do
expected = <<-HTML.strip_heredoc
<div class="form-group">
Expand Down Expand Up @@ -303,22 +329,26 @@ class BootstrapFormGroupTest < ActionView::TestCase
assert_equivalent_xml expected, output
end

test 'form_group renders the "error" class and message corrrectly when object is invalid' do
@user.email = nil
assert @user.invalid?

output = @builder.form_group :email do
%{<p class="form-control-static">Bar</p>}.html_safe
end

expected = <<-HTML.strip_heredoc
<div class="form-group">
<p class="form-control-static">Bar</p>
<div class="invalid-feedback">can't be blank, is too short (minimum is 5 characters)</div>
</div>
HTML
assert_equivalent_xml expected, output
end
# test 'form_group renders the "error" class and message corrrectly when object is invalid' do
# # It could be said that the meaning of "form-group" has changed in Bootstrap 4,
# # and that's why it shouldn't be outputting the error message anymore. Which
# # would make this test case no longer valid.
# # THIS TEST WAS REMOVED FROM v2.7.
# @user.email = nil
# assert @user.invalid?
#
# output = @builder.form_group :email do
# %{<p class="form-control-static">Bar</p>}.html_safe
# end
#
# expected = <<-HTML.strip_heredoc
# <div class="form-group">
# <p class="form-control-static">Bar</p>
# <div class="invalid-feedback">can't be blank, is too short (minimum is 5 characters)</div>
# </div>
# HTML
# assert_equivalent_xml expected, output
# end

test "adds class to wrapped form_group by a field" do
expected = <<-HTML.strip_heredoc
Expand Down
10 changes: 5 additions & 5 deletions test/bootstrap_radio_button_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ class BootstrapRadioButtonTest < ActionView::TestCase
<label class="form-check-label" for="user_misc_1">
Foobar
</label>
<small class="form-text text-muted">With a help!</small>
</div>
<small class="form-text text-muted">With a help!</small>
</div>
HTML

Expand Down Expand Up @@ -172,8 +172,8 @@ class BootstrapRadioButtonTest < ActionView::TestCase
<div class="form-check">
<input class="form-check-input" id="user_misc_1" name="user[misc]" type="radio" value="1" />
<label class="form-check-label" for="user_misc_1"> rabooF</label>
<small class="form-text text-muted">With a help!</small>
</div>
<small class="form-text text-muted">With a help!</small>
</div>
HTML

Expand All @@ -188,8 +188,8 @@ class BootstrapRadioButtonTest < ActionView::TestCase
<div class="form-check">
<input class="form-check-input" id="user_misc_address_1" name="user[misc]" type="radio" value="address_1" />
<label class="form-check-label" for="user_misc_address_1"> Foobar</label>
<small class="form-text text-muted">With a help!</small>
</div>
<small class="form-text text-muted">With a help!</small>
</div>
HTML

Expand Down Expand Up @@ -242,8 +242,8 @@ class BootstrapRadioButtonTest < ActionView::TestCase
<div class="form-check">
<input class="form-check-input" id="user_misc_1" name="user[misc]" type="radio" value="1" />
<label class="form-check-label" for="user_misc_1"> rabooF</label>
<small class="form-text text-muted">With a help!</small>
</div>
<small class="form-text text-muted">With a help!</small>
</div>
HTML

Expand All @@ -258,8 +258,8 @@ class BootstrapRadioButtonTest < ActionView::TestCase
<div class="form-check">
<input class="form-check-input" id="user_misc_address_1" name="user[misc]" type="radio" value="address_1" />
<label class="form-check-label" for="user_misc_address_1"> Foobar</label>
<small class="form-text text-muted">With a help!</small>
</div>
<small class="form-text text-muted">With a help!</small>
</div>
HTML

Expand Down
Loading