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

Support nil value for label parameter #457

Closed
santiagodoldan opened this issue Apr 11, 2018 · 8 comments
Closed

Support nil value for label parameter #457

santiagodoldan opened this issue Apr 11, 2018 · 8 comments

Comments

@santiagodoldan
Copy link

Looks like passing nil or an empty string to the label parameter does not remove the text, it shows the attribute name, I was wondering if it could be a good idea to support nil to remove that text, in my case I was trying to use the check_box helper method and trying to integrate it with a bootstrap switch plugin which only uses css, so it needs a label without content.

If this is something that can be merge, I'd love to create a PR for it.

Here an example

= f.check_box :name

<div class="form-check">
  <input name="name" type="hidden" value="0">
  <input class="form-check-input" type="checkbox" value="1" name="name" id="name">
  <label class="form-check-label" for="name">Active</label>
</div>

= f.check_box :name, label: nil

<div class="form-check">
  <input name="name" type="hidden" value="0">
  <input class="form-check-input" type="checkbox" value="1" name="name" id="name">
  <label class="form-check-label" for="name"></label>
</div>
@lcreid
Copy link
Contributor

lcreid commented Apr 11, 2018

Thanks for the suggestion, Santiago. I see what the problem is. My only concern with the proposed solution is that it might break existing code being migrated from the Bootstrap 3 version of this gem. I assume from your example that you're using Bootstrap 4.

I checked the behaviour of the Bootstrap 3 version of the gem. If you specify label: "" to the Bootstrap 3 version, it gives an empty label, which I think is what you want.

I did a fair bit of work in check_box and radio_button for the Bootstrap 4 version, and it looks like I introduced a regression. If you want to work on a PR to fix the regression, and make it so label: "" gives and empty label tag, please say so in a comment on this issue. Otherwise, I'll take a look at it, but probably not until the weekend.

Thanks again for raising this issue!

@GBH
Copy link
Contributor

GBH commented Apr 11, 2018

@santiagodoldan this is due to how form.label works in Rails. See: http://api.rubyonrails.org/v5.2/classes/ActionView/Helpers/FormHelper.html#method-i-label

nil and "" are effectively blank? so helper falls back to the method name. You could try to use I18n for it (not sure if that will work though)

Easy work-around: label: "&nbsp;".html_safe. That will render label with no content (non-breakable space actually)

@lcreid
Copy link
Contributor

lcreid commented Apr 15, 2018

The reason label: "" generated a label tag with no text in bootstrap_form 2.7 is because in Bootstrap 3 markup wrapped the check box or radio button inside the label. Bootstrap 4 requires the control to be outside the label. As noted in #457 (comment), the bootstrap_form 2.7 behaviour is arguably inconsistent with the behaviour of the underlying Rails label helper.

For the issue of how to generate an empty label, I see three possible solutions:

  1. Maintain consistency with bootstrap_form 2.7 and make it so label: "" (or label: nil, or both) generate an empty label. This avoids breaking old code, but means that bootstrap_form arguably is inconsistent with the underlying Rails helpers
  2. Add a empty_label: true option to check_box and radio_button to generate a visible but empty label. Programmers who had used the bootstrap_form 2.7 label: "" will have to change their code, but the change is easy. It makes the intent of the code clear, at the cost of adding yet another option to check_box and radio_button
  3. Provide a few examples in the documentation about how to use the underlying Rails helpers with Bootstrap 4 markup, so that users faced with cases like this can see how edge cases like this can be handled without having to abandon bootstrap_form altogether

I don't see any of these as a clear winner, but my preference is for 2., then 3., then 1. @mattbrictson (and anyone else), your thoughts?

@santiagodoldan
Copy link
Author

I agree that a new option would be better, empty_label: true makes sense to me

@lcreid
Copy link
Contributor

lcreid commented Apr 23, 2018

@santiagodoldan Let us know if you plan to send a PR for this. We'd love to have your contribution and would be very thankful for a PR.

@santiagodoldan
Copy link
Author

Sure thing, will try to add it this week

@mattbrictson
Copy link
Contributor

I'm late to this discussion, but if I may play devil's advocate for a bit?

We already have skip_label: true. This omits the label entirely. We also have hide_label: true. This hides the label but keeps the text for screen readers.

Do either of those work for your use case? I am not sure why you'd want to keep a <label> element in the markup with nothing inside of it. But, following on the previous suggestion, you could instead set a label of a "zero width space" which has an HTML entity of &#8203;.

label: "&#8203;".html_safe

Or maybe less esoteric:

label: "<span></span>".html_safe

I guess my point is that adding another :empty_label option to an already confusing array of label options may not be worth the trouble. If Rails itself does not allow "" as a label value, then probably this is not a very common problem?

Could we add one of these workarounds to the UPGRADE-4.0 doc instead?

@lcreid
Copy link
Contributor

lcreid commented May 29, 2018

I was reluctant to add yet another option, but the Santiago's use case requires an empty label to work with another plugin. But that's arguably enough of an edge case that I now agree that we should just document the "zero width space" trick.

@santiagodoldan could you please look at Matt's comment and see if it's good enough for your use case?

lcreid added a commit to lcreid/rails-bootstrap-forms that referenced this issue Jun 3, 2018
lcreid added a commit to lcreid/rails-bootstrap-forms that referenced this issue Jun 4, 2018
lcreid added a commit to lcreid/rails-bootstrap-forms that referenced this issue Jun 5, 2018
lcreid added a commit to lcreid/rails-bootstrap-forms that referenced this issue Jun 5, 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

No branches or pull requests

4 participants