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

Testing form_with and Rails Versions #451

Open
lcreid opened this issue Mar 12, 2018 · 5 comments
Open

Testing form_with and Rails Versions #451

lcreid opened this issue Mar 12, 2018 · 5 comments
Labels

Comments

@lcreid
Copy link
Contributor

lcreid commented Mar 12, 2018

[This issue is for discussion and to record information that may be useful to future maintainers.]

This PR adds a new helper bootstrap_form_with to support the Rails 5.1 form_with helper (Issue #326). Support for bootstrap_form_with was added by PR #369.

The code changes to the helpers to support bootstrap_form_with were not extensive, but testing was a challenge. form_with adds to the variety of expected outputs that tests have to handle. A couple of varieties of testing approaches were proposed in early versions of PR #369, and in #346, but all were deemed too complicated, or had too much duplication. In the end #369 includes a few tests to check specific form_with related functionality, but otherwise testing is left to form_for and form_tag.

If, as was announced originally, form_with will eventually replace form_for and form_tag, we'll have to update our tests. Originally, I thought it would be best to provide a complete suite of form_with tests now, while one has their head in the problem.

However, with Rails 5.2 on the horizon, I would propose that we do not increase test coverage for form_with until at least Rails 5.2, and then basically ignore testing of Rails 5.1 beyond what we currently have. This will eliminate one dimension of test variability. Also, my experience trying to use form_with in Rails 5.1 was sufficiently painful that I think we would be doing people a favour by encouraging them to switch to Rails 5.2 if they want to use form_with.

Eventually, we'll have to do all testing against form_with, but hopefully by then a number of the other variations in test will no longer be needed, because those versions are no longer supported. So testing should be easier at that time.

For the record, here are some examples of the differences in output that have accumulated over time (not just due to #369):

  • Prior to Rails 5, HTML tags were output in alphabetical order. After, they weren't
  • Some helpers starting with Rails 5 produced more output, e.g. an empty hidden input on file tags (The genesis of assert_equivalent_xml is found in Rewrite tests to work on Rails 4.0, 4.1, 4.2, and 5.0 #273 and Testing multiple versions of Rails #278, which specifically addressed these first two issues.)
  • Rails 5.1. stopped generating default DOM ids in the form helpers, and stopped putting the new_... HTML class on forms. There may be other differences that I've forgotten already
  • Rails 5.2 returns to putting default DOM ids, but still doesn't put the HTML class on forms
  • form_with does submits via unobtrusive Javascript, so form tags are marked with data-remote="true".

(https://m.patrikonrails.com/rails-5-1s-form-with-vs-old-form-helpers-3a5f72a8c78a is an easy-to-read summary of differences between form_for and form_with.)

@GBH
Copy link
Contributor

GBH commented Mar 13, 2018

Is this the place to discuss testing approach?

Here's how test suite could be written to be simple and manageable.

There's absolutely no difference between Rails 5.0, 5.1 and 5.2 as far as ActionView::Helpers::FormBuilder is concerned. The only difference is how it's being initialized by view helpers like form_for or form_with. So all tests that test builder methods like text_field should do it directly via an instance of a builder, not via html generated by form_for view helpers.

Then you can create a separate test file that tests view helpers only. Something like assertion of output for:

bootstrap_form_for @record do |form|
  form.text_field :foo
end

Output of that may differ between Rails versions. However you do not test every builder method like that as you're effectively triplicating test suite for no reason.

@gafemoyano
Copy link

So the only issue I've encountered when submitting a bootstrap_form_with remotely is that the submit button gets enabled before the page starts transitioning via Turbolinks.

This makes it so users can submit the form multiple times while waiting for Turbolinks to vist the new URL.

Is there any way around this, could anybody point me on the right direction to stop this rom happening?

Thank you.

@Xavoski
Copy link

Xavoski commented Dec 2, 2018

Option "size" in form.text_field isn't working for bootstrap_form_with.
<%= form.text_field :name, size: 40 %>
<%= form.text_field :name, size: 20 %>

If the code is like above, both fields will be the size of bigger one (40).
Do you recommend another way to define the size of text_field?

@lcreid
Copy link
Contributor Author

lcreid commented Dec 4, 2018

@Xavoski thanks very much for the feedback. We need to hear from users to make bootstrap_form better. Sorry for my delay in responding.

I'm having trouble reproducing your issue, but I have one guess as to what might be happening: Bootstrap expands input fields to the full width of the enclosing element. Depending on the HTML that you have around your text fields, that might account for why they both look like they have the same width.

Bootstrap puts a width: 100% in the form-control class, which is the class that styles the form input fields with the Bootstrap look and feel. You could try putting , style: "width: auto;" after the size in each of your fields. (I tried adding the Bootstrap class w-auto to the fields, but that didn't seem to work. Maybe you can figure out why w-auto didn't work.)

Let me know if this helps! And thanks again for your feedback.

@Xavoski
Copy link

Xavoski commented Dec 5, 2018

After your sugestion, style: "width: auto;", text_field are respecting the option size: 20 and fields are not using the width from form-control class. So, problem was solved.
Thanks for your attention and congratulions for this gem.

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

No branches or pull requests

4 participants