Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

cleanup Labelling#label_html_options #865

Merged
merged 1 commit into from

5 participants

@jpmckinney

Labelling#label_html_options has some rather strange and unnecessary logic just to spit out a hash. Cleaned up.

@justinfrench
Owner

This would prevent users from setting their own :class or :for attribute. I agree the code is a complicated mess, but your implementation is missing some important stuff (and our tests?).

Was this to address a bug, or just a refactor attempt?

@jpmckinney

Let me go through that method's body line-by-line - maybe I'm missing something:

  1. # opts = options_for_label(options) # TODO

    This is a comment. Does nothing.

  2. opts = {}

    Sets opts to {}.

  3. opts[:for] ||= input_html_options[:id]

    opts is {} so the ||= is the same as =. Sets opts to {:for => input_html_options[:id]}.

  4. opts[:class] = [opts[:class]]

    opts[:class] is not set on right-hand side. All this does is add a :class key set to [nil].

  5. opts[:class] << 'label'

    This makes opts equal to {:for => input_html_options[:id], :class => [nil, 'label']}

  6. opts

    returns {:for => input_html_options[:id], :class => [nil, 'label']}

And that's exactly what my refactor returns, except without the nil class (which I assume does nothing?)

@justinfrench
Owner

@jpmckinney you're right, sorry. The refactor is fine, now I think you've highlighted a regression. I'll look into it when I can, thanks, will get back to you soon.

@jpmckinney

Seems to have been the same since the introduction of label_html_options in d5a035c

d5a035c#L3R51

@justinfrench
Owner

@jpmckinney Looks like we used to support :label_html as an option, but there's no evidence in the code any more, no mention in the tests, looks like it got dropped in the big refactor and no one noticed. Merging!

@justinfrench justinfrench merged commit 0f2b83e into justinfrench:master
@jtomaszewski jtomaszewski referenced this pull request in mjbellantoni/formtastic-bootstrap
Closed

Thoughts on using form-horizontal? #34

@jtomaszewski

Why there's no support for :label_html option? I think it could be useful in some uses (f.e. achieving the form-horizontal look in Boostrap3). Shall I create an issue/pull-request with it?

@jtomaszewski jtomaszewski referenced this pull request in mjbellantoni/formtastic-bootstrap
Merged

form-horizontal support for Bootstrap 3.x #94

@felixbuenemann

@jtomaszewski It's probably better to open a new issue to discuss this and just reference this one.

@RajRoR

@jpmckinney How I can avoid having 'label' as class for my all labels in semantic_form_for?

I am using bootstrap, so bootstrap defines 'label' class as:

.label {
  display: inline;
  padding: .25em .6em;
  font-size: 75%;
  font-weight: bold;
  line-height: 1;
  color: #ffffff;
  text-align: center;
  white-space: nowrap; 
  vertical-align: baseline;
  border-radius: .25em;
}

Now, as you can see, its making labels text WHITE. This is my problem. I just want to avoid having label class.

@RajRoR You should open an issue on Formtastic. I'm not the maintainer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 3, 2012
  1. @jpmckinney
This page is out of date. Refresh to see the latest.
Showing with 4 additions and 7 deletions.
  1. +4 −7 lib/formtastic/inputs/base/labelling.rb
View
11 lib/formtastic/inputs/base/labelling.rb
@@ -10,13 +10,10 @@ def label_html
end
def label_html_options
- # opts = options_for_label(options) # TODO
- opts = {}
- opts[:for] ||= input_html_options[:id]
- opts[:class] = [opts[:class]]
- opts[:class] << 'label'
-
- opts
+ {
+ :for => input_html_options[:id],
+ :class => ['label'],
+ }
end
def label_text
Something went wrong with that request. Please try again.