cleanup Labelling#label_html_options #865

Merged
merged 1 commit into from Jul 3, 2012

Conversation

Projects
None yet
5 participants
Contributor

jpmckinney commented Jul 3, 2012

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

Owner

justinfrench commented Jul 3, 2012

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?

Contributor

jpmckinney commented Jul 3, 2012

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

Owner

justinfrench commented Jul 3, 2012

@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.

Contributor

jpmckinney commented Jul 3, 2012

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

d5a035c#L3R51

Owner

justinfrench commented Jul 3, 2012

@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 added a commit that referenced this pull request Jul 3, 2012

@justinfrench justinfrench merged commit 0f2b83e into justinfrench:master Jul 3, 2012

@jtomaszewski jtomaszewski referenced this pull request in mjbellantoni/formtastic-bootstrap Jan 31, 2014

Closed

Thoughts on using form-horizontal? #34

Contributor

jtomaszewski commented Jan 31, 2014

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 Jan 31, 2014

Merged

form-horizontal support for Bootstrap 3.x #94

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

@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.

Contributor

jpmckinney replied Aug 20, 2014

@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