Skip to content

Conversation

duleorlovic
Copy link
Contributor

Move process_options from helper since we can not override helper.
When someone wants to extend BootstrapForm::FormBuilder and use it in
form_with .... builder: MyFormBuilder, before this change he could not
use in this way since determination whether it is :inline :horizontal
was done in helper.

Move `process_options` from helper since we can not override helper.
When someone wants to extend `BootstrapForm::FormBuilder` and use it in
`form_with .... builder: MyFormBuilder`, before this change he could not
use in this way since determination whether it is :inline :horizontal
was done in helper.
@bootstrap-ruby-bot
Copy link

1 Warning
⚠️ Please update CHANGELOG.md with a description of your changes. If this PR is not a user-facing change (e.g. just refactoring), you can disregard this.

Here's an example of a CHANGELOG.md entry (place it immediately under the * Your contribution here! line):

* [#550](https://github.com/bootstrap-ruby/bootstrap_form/pull/550): Add `default_layout` so we can use it for all forms - [@duleorlovic](https://github.com/duleorlovic).

Generated by 🚫 Danger

Copy link
Contributor

@lcreid lcreid left a comment

Choose a reason for hiding this comment

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

Good to hear from you again. This is a nice PR. I like your idea. It makes more sense to put the option in the builder itself, rather than the helper.

I don't understand why you had to change the test cases you had to change. If you know why, please let me know. I would think that moving the place where we set the option should require no changes to our tests. If I get some time I'll try to figure it out, unless you answer first.

As soon as we understand why the tests changed, we'll look at merging this PR.

@duleorlovic
Copy link
Contributor Author

The change in test is only about adding class new_user which rails adds automatically for form_for syntax if html: { class: } were not given in apply_form_for_options (please note reverse_merge and actual source for new_name class).

So I need to change only those tests that were using layout: :inline option for form_for since now we add form-inline in builder (not as html: { class: 'form-inline' } option for form_for).

If you like, I replaced those tests with newer form_with syntax and than we do not have those additional classes. I will revert last commit if form_with syntax is not needed.

I think it is safe to merge since this pull request will only add new_user class for layout: :inline forms (so form will have new_user form-inlne instead of form-inline class) and only for old form_for syntax.

@duleorlovic
Copy link
Contributor Author

I reverted Use form_with syntax since is it not available on Rails < 5.1...

@duleorlovic duleorlovic force-pushed the add_default_layout_method branch from ef1d640 to bc283da Compare November 6, 2019 09:04
Copy link
Contributor

@lcreid lcreid left a comment

Choose a reason for hiding this comment

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

Sorry for the slow response. Unfortunately, I don't use this gem in my day job, so it can be a struggle at times to find time to work on it.

I played around with your PR a bit and looked at what it's doing, and I'm pretty much convinced that in fact you've found some tests that were "wrong" by my criteria. I want to do a bit more research to see when the wrong tests were introduced.

Please be patient. I will get to this soon. Thanks again for the contribution.

@lcreid
Copy link
Contributor

lcreid commented Nov 20, 2019

I am comfortable that the change in test cases introduced by the PR is in fact the way they should be. If we break any existing users' code (which I think will be in relatively few cases), I'm comfortable defending it as a fix to code that was broken.

@duleorlovic I'll merge this now, but please confirm that we don't need to change any documentation (I don't think so, but please forgive me for double-checking.)

@lcreid lcreid merged commit 13b825a into bootstrap-ruby:master Nov 20, 2019
@duleorlovic duleorlovic deleted the add_default_layout_method branch November 20, 2019 05:07
@duleorlovic
Copy link
Contributor Author

I think example in README is enough https://github.com/bootstrap-ruby/bootstrap_form/pull/550/files#diff-04c6e90faac2675aa89e2176d2eec7d8R624

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.

3 participants