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

Handle inline radio buttons and check boxes for Bootstrap 4. #410

Merged
merged 2 commits into from
Jan 29, 2018

Conversation

lcreid
Copy link
Contributor

@lcreid lcreid commented Jan 28, 2018

The Bootstrap 4 tags for radio buttons and check boxes is more regular, so the helpers are somewhat shorter, at the cost of perhaps making the review more difficult.

If there's a way to show the diff without comparing white space, it might give you a better idea of the real changes, most notably in the test cases.

This PR partially addresses #395.

@lcreid lcreid requested review from mattbrictson and donv January 28, 2018 21:43
@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):

* [#410](https://github.com/bootstrap-ruby/bootstrap_form/pull/410): Handle inline radio buttons and check boxes for Bootstrap 4. - [@lcreid](https://github.com/lcreid).

Generated by 🚫 Danger

@mattbrictson
Copy link
Contributor

FYI you can append ?w=1 to a PR diff to ignore white space, like this: https://github.com/bootstrap-ruby/bootstrap_form/pull/410/files?w=1

Copy link
Contributor

@mattbrictson mattbrictson left a comment

Choose a reason for hiding this comment

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

Looks great. Nice code cleanup ✨

I do however see an issue that maybe warrants a separate issue or PR. Our documentation is not clear on how you properly create an inline form.

Some parts of the README imply that you just add layout: :inline to bootstrap_form_for and you are done. But that is either broken or the doc is wrong, because as I see in the tests, you must explicitly pass inline: true to each radio or checkbox (in addition to passing layout: :inline for the form?).

Furthermore the inline example in demo/ makes the same mistake, and the inline checkboxes there do not render nicely as a result.

I think we need to either change both the demo and the README, or our radio/check helpers need to perhaps assume inline: true if they are within a form that is layout: :inline. The latter sounds like it could be harder to get right.

@lcreid
Copy link
Contributor Author

lcreid commented Jan 29, 2018

Nice catch. I was so into just mechanically changing the existing test cases that I didn't notice that we weren't testing the case where the form has the layout: inline. In the spirit of small, bite-sized PRs, I would humbly suggest we merge this one, and raise a new issue for the form-level inline. I won't be putting in an issue tonight, but I can do it in the morning if you haven't already done so.

@mattbrictson
Copy link
Contributor

Sounds good! If you could open the issue tomorrow I'd appreciate it.

@mattbrictson mattbrictson merged commit dbaa1f8 into bootstrap-ruby:master Jan 29, 2018
@lcreid lcreid deleted the 395-inline branch January 29, 2018 05:03
lcreid added a commit to lcreid/rails-bootstrap-forms that referenced this pull request Jun 4, 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.

3 participants