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

Render Bootstrap 4 Radio Buttons and Check Boxes #407

Merged
merged 7 commits into from
Jan 28, 2018

Conversation

lcreid
Copy link
Contributor

@lcreid lcreid commented Jan 27, 2018

Bootstrap 4 no longer wraps radio buttons and check boxes inside the label. Previously it was:

<div class="form-check">
  <label class="form-check-label" for="user_misc_1">
    <input class="form-check-input" type="checkbox" value="1" name="user[misc][]" id="user_misc_1" />
    Foo
  </label>
</div>

<div class="radio">
  <label for="user_misc_1">
    <input type="radio" value="1" name="user[misc]" id="user_misc_1" />
    Foo
  </label>
</div>

Where it should be:

<div class="form-check">
  <input class="form-check-input" type="checkbox" value="" id="defaultCheck1">
  <label class="form-check-label" for="defaultCheck1">
    Default checkbox
  </label>
</div>

<div class="form-check">
  <input class="form-check-input" type="radio" name="exampleRadios" id="exampleRadios1" value="option1" checked>
  <label class="form-check-label" for="exampleRadios1">
    Default radio
  </label>
</div>

I really couldn't see any sensible way to change the rendering without doing a little bit of simplifying of the code in the radio_button_with_bootstrap and check_box_with_bootstrap helpers. I suspect more refactoring/simplification could be done in those methods, but I thought now is not the time to do that.

I would appreciate it if someone who's a little more knowledgeable about Bootstrap 4, makes sure that I changed the test cases correctly.

@bootstrap-ruby-bot
Copy link

bootstrap-ruby-bot commented Jan 27, 2018

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):

* [#407](https://github.com/bootstrap-ruby/bootstrap_form/pull/407): Render Bootstrap 4 Radio Buttons and Check Boxes - [@lcreid](https://github.com/lcreid).

Generated by 🚫 Danger

Copy link
Collaborator

@donv donv left a comment

Choose a reason for hiding this comment

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

This PR looks good to me. I will start using it in my apps as soon as it is merged.

@mattbrictson
Copy link
Contributor

The general checkbox case looks good, but I am seeing a couple minor discrepancies otherwise.

The Bootstrap v4 docs say that radios should be wrapped in <div class="form-check">, but our test case has <div class="radio">.

Also, for disabled checkboxes, the docs say the wrapper should be <div class="form-check"> but we have <div class="form-check disabled">. The disabled class is unnecessary. (Interestingly, the disabled class is necessary for wrapping disabled radios.)

@mattbrictson
Copy link
Contributor

Also: It looks like inline checkboxes might need a separate PR?

@lcreid
Copy link
Contributor Author

lcreid commented Jan 28, 2018

OK. I need to step up my game on reading the Bootstrap docs.

Now that I'm starting to do that, I see that radio buttons should also have class form-check-input on the input tags, and form-check-label on the label tags. We have them on the check box test cases, but not the radio button test cases. Does anyone else read it that way?

It also looks like I should be handling the "no label" case explicitly: https://getbootstrap.com/docs/4.0/components/forms/#without-labels. I think I'll do that in a separate PR.

@mattbrictson what did you mean by inline checkboxes needing a separate PR? Is it because both radio and check boxes inline are rendered differently compared to Bootstrap 3, and therefore the changes are more extensive?

I will finish implementing the two fixes @mattbrictson mentioned in his comment just above and await answers to some of these questions.

Needed to use `disabled="disabled"` form of attribute for Nokogiri
and/or EquivalentXML.
@lcreid
Copy link
Contributor Author

lcreid commented Jan 28, 2018

This seems to show that radio button <div class="form-check"> elements do not need the disabled class, like check boxes. I'll leave the class in the code for now.

@lcreid
Copy link
Contributor Author

lcreid commented Jan 28, 2018

I pushed fixes for #407 (comment) to this PR. I will open other PRs to address each of:

  • correcting the classes on the radio button elements (Render Bootstrap 4 Radio Buttons and Check Boxes #407 (comment)). I'd like confirmation that I'm doing the right thing by adding the classes. I'm now confident enough, however, that I've done the work and have code ready for a PR
  • inline radios and checkboxes
  • custom controls -- Looks like this was already done. Let me know if you see any discrepancies between our output and the Bootstrap 4 documentation
  • update the README where the examples of generated markup are out-of-date
  • the "no label" case, perhaps including an intelligent default for aria-label

@lcreid
Copy link
Contributor Author

lcreid commented Jan 28, 2018

I have code ready to go for PRs for correcting the classes on radio button elements, and for doing inline radios and checkboxes. @mattbrictson I appreciate that the more changes you have to look at, the harder the review, but... If I add the code to finish the radio button classes to this PR, it will be easier for you to reconcile the Bootstrap docs with the PR. Your call whether I add more to this PR or not.

I'll wait until we merge this PR before I push the PR for inline elements.

@mattbrictson
Copy link
Contributor

This looks good to merge, thanks! 💯

I'm fine with handling the remaining cases as separate PRs.

@mattbrictson mattbrictson merged commit 66df980 into bootstrap-ruby:master Jan 28, 2018
@mattbrictson mattbrictson added this to the 4.0.0 milestone Jan 28, 2018
@lcreid lcreid deleted the 395 branch January 28, 2018 22:41
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.

4 participants