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

Update hide_label and skip_label for check boxes and radio buttons #414

Merged
merged 12 commits into from
Feb 12, 2018

Conversation

lcreid
Copy link
Contributor

@lcreid lcreid commented Jan 29, 2018

This PR updates markup for check boxes and radio buttons to Bootstrap 4. It applies to the radio and check_box helpers only, not the collection_radio_buttons and collection_check_boxes. The collection versions should probably be discussed under #412.

Further clean-up of this code is possible, but an attempt was made to minimize changes (not always successful, mind you).

This is part of the response to #395.

@lcreid lcreid requested review from donv and mattbrictson January 29, 2018 19:54
@mattbrictson
Copy link
Contributor

I think this looks great. The important part is that we have good test coverage for these new cases, which you have done. Thanks!

One question I have is whether position-static should be added for the :hide_label case. The Bootstrap docs don't seem clear on that. Does it look better one way or the other when rendered?

@lcreid
Copy link
Contributor Author

lcreid commented Jan 31, 2018

I convinced myself that position-static wasn't needed for the hide_label case. I believe I did one simple test on bootply.com, but I don't remember for sure. I'll try to find time to do a more careful test.

@lcreid
Copy link
Contributor Author

lcreid commented Feb 5, 2018

@mattbrictson good call on questioning me about hide_label! It does need position_static, as I hope this shows. Without position_static, the buttons pile up on each other, and also on any other text or controls you have adjacent. So I've pushed an update and hopefully we can merge this soon. Again, thanks for catching that.

@lcreid
Copy link
Contributor Author

lcreid commented Feb 5, 2018

BTW, I'm pretty sure that hide_label and skip_label in legacy don't hide or skip the label on a check box or radio button, at least when specified to check_box and radio_button. I checked out legacy at one point, duplicated the basic check box test to two new tests, and added hide_label: true to one of the new tests, and skip_label: true to the other, and they both render labels with no apparent markup that would hide them.

It looks like this was discussed in #161. Someone suggested it wasn't needed because, at least for skip_label, you could use check_box_without_bootstrap. However, with the Bootstrap 4 markup, I think it would be nicer for the user if skip_label and hide_label both do what they're advertised to do.

I guess that means this might be considered a breaking change, since someone upgrading might get quite different markup compared to v2.7. At the very least, probably something for the "Upgrading" document.

It looks like this is the source of a number of issues: #162, #161, and #149.

@lcreid lcreid merged commit 5c6a27c into bootstrap-ruby:master Feb 12, 2018
@lcreid lcreid deleted the 395-hide-label branch February 12, 2018 15:34
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.

2 participants