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

default form-inline class applied to parent content div on date select helpers #461

Merged
merged 2 commits into from
Sep 13, 2018

Conversation

lancecarlson
Copy link
Contributor

can also override this behavior with skip_inline: true

@lancecarlson
Copy link
Contributor Author

Fixes: #460

@@ -52,7 +52,9 @@ def initialize(object_name, object, template, options)
define_method(with_method_name) do |name, options = {}, html_options = {}|
prevent_prepend_and_append!(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))
html_class = control_specific_class(method_name)
html_class = "#{html_class} form-inline" unless options[:skip_inline]
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that you will need to be more specific here about when to add form-inline. I believe you only want to add it when the form_layout is :horizontal.

@@ -52,7 +52,9 @@ def initialize(object_name, object, template, options)
define_method(with_method_name) do |name, options = {}, html_options = {}|
prevent_prepend_and_append!(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))
html_class = control_specific_class(method_name)
html_class = "#{html_class} form-inline" unless options[:skip_inline]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the :skip_inline option a new option you're proposing? If so, could you please update the CHANGELOG to note the new functionality, and probably add a description of the option in the README.

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.

Thanks for this PR. We really appreciate your contribution. Sorry for my delay in responding. We're all volunteers on this project, so we can't always respond right away.

Since your PR identified a gap in our testing, it would help to have a test case in the PR. You can see in test/bootstrap_selects_test.rb, around line 256, there's the basic date select test, which you could copy and use as a starting point. One trick you can use is that we have an instance variable in all tests set up to build horizontal forms. It's called @horizontal_builder. You can use it in place of @builder and update the expected HTML to create a test case that fails, then write the code that makes the test case pass, at the same time that all the existing test cases pass.

To run the tests, type: bundle exec rake. I just noticed that somehow the instructions on how to run tests are not that obvious in our CONTRIBUTING document. Sorry about that.

@lancecarlson
Copy link
Contributor Author

lancecarlson commented May 4, 2018

@lcreid I have no problem writing tests, but I'm having a hard time getting them to run. It keeps saying this:

ActiveRecord::StatementInvalid: Could not find table 'users'

Ah, I just reversed the order of the contributing instructions by running the schema load command first and I go past this error. Maybe we want to reverse the order in the contributing doc?

@lcreid
Copy link
Contributor

lcreid commented May 4, 2018

Yes. I just figured that out myself. I'll fix the documentation.

@lcreid
Copy link
Contributor

lcreid commented May 4, 2018

Documentation fix in PR #462.

@lancecarlson
Copy link
Contributor Author

@lcreid I updated the changelog in this latest commit, but I'm not entirely sure where the skip_inline on the field helper should be documented. I only put it there so people could override if that wasn't the behavior they wanted.

@lcreid
Copy link
Contributor

lcreid commented May 19, 2018

Sorry for not following up on this sooner! This is looking good, but it occurs to me that we should do the same for time_select and datetime_select. Would you mind adding those to this PR similarly to the way you did date_select? Sorry I didn't mention this sooner.

@lcreid
Copy link
Contributor

lcreid commented Jul 18, 2018

@lancecarlson Could you please resolve the conflicts in this PR.

git fetch upstream
git checkout master
git rebase upstream/master

Fix the conflicts and commit the changes. Then just:

git push

Github will update the pull request.

@lcreid
Copy link
Contributor

lcreid commented Jul 18, 2018

@lancecarlson Please let me know if you're going to add this functionality for time_select and datetime_select. I can do it if you don't have time. Just let me know, please. Thanks in advance!

@lcreid lcreid requested a review from mattbrictson July 20, 2018 16:15
@lcreid
Copy link
Contributor

lcreid commented Jul 20, 2018

@lancecarlson thanks for the PR! My apologies for not noticing sooner that you had extended it to cover all the date and time selects. I resolved the conflicts.

@mattbrictson can you please take a quick look at this to make sure the PR doesn't add functionality that it shouldn't? The Bootstrap 4 version was stacking the fields for date and time, rather than putting them in the more intuitive side-by-side #460.

@lcreid lcreid merged commit e54fe9b into bootstrap-ruby:master Sep 13, 2018
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.

2 participants