Skip to content

Conversation

@andrewhavens
Copy link

This PR implements a helper method for generating a Bootstrap label: http://getbootstrap.com/components/#labels

I didn't realize until after I had finished that Rails already has a helper method called label. The weird thing is that it should only be available in the context of form_for:

<%= form_for :person do |f| %>
  <%= f.label :name %>
  <%= f.text_field :name %>
<% end %>

The alternative, which can be used outside of form_for is supposed to be label_tag, so I don't know whylabel is bleeding into the global scope.

Anyway, I just decided to prefix the helper name with bh. What do you think?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling f91f752 on andrewhavens:feature/labels into 49db322 on Fullscreen:master.

@claudiofullscreen
Copy link
Contributor

@andrewhavens You can already do that with Bh.

Any field helper inside the Bh form_for helper automatically gets the label, e.g.:

<%= form_for :user, layout: :horizontal do |f| %>
  <%= f.email_field :email %>
<% end %>

becomes

<form action="/users/sign_in" class="form-horizontal" role="form">
  <div class="form-group">
    <label class="col-sm-3 control-label" for="user_email">Email</label>
    <div class="col-sm-9">
      <input class="form-control" id="user_email" name="user[email]" placeholder="Email" size="30" type="email">
    </div>
  </div>
</form>

If you want to use a different label, just pass it as an optional parameter to the field helper, e.g.:

<%= form_for :user, layout: :horizontal do |f| %>
  <%= f.email_field :email, label: 'Your email' %>
<% end %>

See more examples at http://fullscreen.github.io/bh/#forms. Does that work for you?

@andrewhavens
Copy link
Author

I think you misunderstood. This helper is for creating a Bootstrap label, not a form label. See http://getbootstrap.com/components/#labels

@andrewhavens
Copy link
Author

I should have provided an example in this pull request.

<%= bh_label 'Hello World', context: :primary %>

generates:

<span class="label label-primary">Hello World</span>

@claudiofullscreen
Copy link
Contributor

@andrewhavens I'm sorry, I misunderstood 😩

The reason why there is yet no helper for label is explained here:
#15 (comment)

What do you think?

@andrewhavens
Copy link
Author

@claudiofullscreen Makes sense. The HTML is relatively simple. The reason I created it was to address a different problem.

Writing this is a lot more work:

<% if thing.needs_a_label? %>
  <span class="label label-default"><%= thing.name %></span>
<% end %>

Than writing this:

<%= label thing.name if thing.needs_a_label? %>

It's also easier to read and avoids having to manually update the html or classes if Bootstrap ever decided to change. Using bh helps me write more readable code and protects me from "Bootstrap API changes".

@claudiofullscreen
Copy link
Contributor

@andrewhavens Thanks for your comment and for using Bh!

As I wrote in the other comment, the reason why I'm holding off on this is not because I don't believe the code might be shorter or more readable, but it's because I'm not 100% sure about the consequences that would bring. Here are some of my unanswered questions.

As you found out, adding a helper called label might start interfering with Rails' helpers… should I start adopting a brand new convention, using the bh_ prefix for every helper?

If I green-light having one helper that barely reduces the amount of HTML, will I start receiving PRs to turn every existing Bootstrap class into a helper? Will that bloat the namespace so much that Bh will be incompatible with other gems?

Since I just started this project, I'd like to be conservative as much as possible. Surely, Bootstrap might change in the future, and the goal of Bh is to "protect" from those changes… but I think that being able to completely forget about any Bootstrap element is currently out of the scope.

An example is the doctype: should I add a helper just to set the DOCTYPE to HTML 5, as Bootstrap requires? Probably doesn't help too much, and obscures the code more than make it clear.


In my personal opinion, there is a clear improvement in legibility when you go from:

<button class="btn btn-default" data-toggle="modal" data-target="#modal-6812963836">
  Modal
</button>
<div class="modal fade" id="modal-6812963836" tabindex="-1" role="dialog" aria-labelledby="label-modal-6812963836" aria-hidden="true" style="display: none;">
  <div class="modal-dialog">
    <div class="modal-content">
      <div class="modal-header">
        <button type="button" class="close" data-dismiss="modal">
          <span aria-hidden="true">×</span><span class="sr-only">Close</span>
        </button>
        <h4 class="modal-title" id="label-modal-6812963836">Modal</h4>
      </div>
      <div class="modal-body">Do what you want!</div>
    </div>
  </div>
</div>

to:

<%= modal body: 'Do what you want!' %>

Back to the example you mentioned, if your final html.erb field needs to look like this:

<% if thing.needs_a_label? %>
  <span class="label label-default"><%= thing.name %></span>
<% end %>

and your things.needs_a_label? method is defined on some property of thing, such as:

# Thing.rb
def needs_a_label?
  updated_at > 1.day.ago
end

then my suggestion is to move the view logic out of the Thing model into your personal helper in app/helpers that prints out a label if a condition is met:

def label_if(caption, condition)
   content_tag :span, caption, class: 'label label-default' if condition
end

so you can have a one-liner like:

<%= label_if thing.name, thing.updated_at > 1.day.ago %>

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