add html classes to inputs with the wrappers API

[stephenprater]

enable a user to add classes or other html attributes to inputs,
labels, and custome components using the wrappers API. this is
accomplished by passing false to the tag key of the wrap_with
options - like so

b.use :input, :wrap_with { :tag => false, :class => ['cool'] }

passing an argument with a false tag is only meaningful in the case
of a single wrapped component which generates a tag itself
(so, label and input is about it out of the box.) A custom
component which wants to support this functionality should look in
options[:#{namespace}_html] for the html options to use.

My particular use case for this is that I have about a dozen different
forms, but only three "looks" - and bootstrap has a few places where
you need to put classes on the input. Rather than have to use
a custom input type, just to add a class to the string input, i want
to create three different wrappers for my three different looks and
specify the wrapper once.

[rafaelfranca]

I think you are distorting the wrappers API. :wrap_with is supposed to wrap an element, and in this case no wrapper are being created, and the classes are being added to the input.

I don't think that we should change the API to do this. I'm not happy with this implementation. Maybe allowing to pass classes to the component would be better. Something like:

b.use :input, :class => ['cool']

[rafaelfranca]

@agrussellknives do you see any easy way to change your pull request to not change the wrappers API and pass the classes directly to the component?

I really don't want to change the wrappers API to this case. Your feature make sense but the wrappers API is complex and using it to change classes without add a wrapper will make more complex.

Also, thank you for the pull request.

[rafaelfranca]

Some historial information. In #457 I change the wrappers API to not use :class and :tag directly to the component. The main reason was: the API was confusing and we don't know when it is creating a wrapper, or if it is using the classes directly.

I think that now we can change the API to pass the options directly to the component.

b.use :input, :class => ['cool']
# Add :class to the input component

b.use :input, :wrap_with => { :class => :cool }
# create a wrapper with the class 'cool' to wrap the input.

WDYT?

cc @carlosantoniodasilva @josevalim

[josevalim]

@rafaelfranca it looks good, but we should be careful because a user my pass some options that are not recognized, for example:

b.use :placeholder, :class => ["cool"]

So we need a form of contract between the underlying input and this.

Finally, if we are improving this area, we should probably deprecate the others (less flexible) ways to customize the input classes.

[stephenprater]

I've got no problem changing it for sure. I prefer @rafaelfranca 's API but didn't want to take it upon my self to un-depreciate things :)

I took my cue from the parameters to the wrapper method - where you can use false as the value for :tag to prevent it from generating a top level tag. The reason that you do THAT (correct me if I'm wrong) is so that you can turn off components (like placeholder) in a wrapped area without generating a surrounding tag correct? Should I change that too or is that an entirely separate issue? It's sort of confusing to me that it's okay in one instance but not in the other - but I don't think I'm fully understanding why it works that way in the the case of wrapper

[carlosantoniodasilva]

@stephenprater you can give false to any component and it won't be included - ie label: false, or error: false.

@rafaelfranca I think it's a good approach to follow, would allow configuration per wrapper instead of global label/input classes for instance. Unsure about the need to deprecate the SimpleForm one's, since we could consider those as global to all wrappers?

[rafaelfranca]

So you can pass :tag => false to wrappers (I don't know if it works with wrapper too) to disable the wrapper element. This make since, since wrappers (and wrapper) is creating a new tag around the block content.

But :wrap_with option is supposed to create a wrapper element around the component that you are passing the option. It is a shortcut to

b.wrapper :tag => 'div', :class => 'foo' do |x|
  x.use :input
end

So we can't use this option to change the component, or we will distort the wrappers API and make it more confusing.

[stephenprater]

@rafaelfranca gotcha. makes sense now.

[rafaelfranca]

@carlosantoniodasilva yes, we can leave the SimpleForm configuration.

[rafaelfranca]

@stephenprater are you work on it? I think this is a good path to add more customization to SimpleForm

[stephenprater]

@rafaelfranca yes, i'll work on it.

[stephenprater]

@josevalim How and how strictly do you think this contract should be enforced? Ignore non-whitelisted attributes? Raise an exception? Print a warning? Any guidance is appreciated.

Current behavior just ignores things that don't make sense (like class on placeholder) and adds non standard HTML attributes for tag type components.

[rafaelfranca]

@stephenprater seems good now. I think is better to use black list in this case. Print a warning if the component is
placeholder, error, hint, html5, maxlength, min_max, pattern and readonly. This will make possible to use this new API with custom components.

[rafaelfranca]

@carlosantoniodasilva @josevalim thoughts?

[rafaelfranca]

why :input is getting a different approach?

[stephenprater]

inputs keep their classes in a separate instance variable. it's duped from @html_classes in SimpleFrom::Inputs::Base#initialize - why it does that I couldn't tell you.

[rafaelfranca]

Ok. Just to confirm. I was not in my computer and didn't look at the code. Thanks to explain.

[stephenprater]

Anything more I need to do to get this merged?

[rafaelfranca]

@stephenprater I'll take a look and merge this tomorrow.

[rafaelfranca]

@carlosantoniodasilva @josevalim @nashby do you guys agree with this path? I will make some refactorings after merge this one

[rafaelfranca]

I think this should be a constant since we are using only in this class

[stephenprater]

plugins, etc can add non tag producing components to wrappers, so if you wanted to be a good simpleform citizen, you'd add your component namespace this this list. I mean, I know you can modify the constant in place, since it's Ruby, but this would allow you to do something like this:

module SimpleForm
  module Components
    module Whatever
       SimpleForm::Wrapper::Builder.non_tag_components << :whatever

       def whatever
         input_html_options[:'data-whatever'] ||= whatever_value
         nil
       end

       def whatever_value
         options[:whatever]
       end
   end
end

That said, I could abstract this so you're not directly addressing the builder class. It seems that theoretically the responsibility to know whether or not it produces a tag should really lie with the component definition - so maybe we move all of this logic there? That sounds like a separate patch though.

[guigs]

Thanks @stephenprater. I'm testing my app with your branch.
In case that the input needs an extra class, input_html: { class: 'extra' }, do you think the output should also contain default class (merged) or we must add both input_html: { class: 'extra form-control' }?

[jrhorn424]

Hate to be a noob, but @stephenprater do you mean rebase against plataformatec's or your pull? And then download the patchfile and apply?

[stephenprater]

I rebased against upstream (this fork) a couple of months ago and can get a clean apply of the patch, but if you want to use my fork just depend on it in your Gemfile (make sure you use the classes_on_use branch)

[jrhorn424]

Thanks, @stephenprater.

[ZenCocoon]

Is this pull request still of actuality? config.input_class have been merged, would be great to get this one as well to tackle the problem with more flexibility. (eg: with Bootstrap 3, have a form-control class on inputs but not checkboxes and radio buttons, which can be nicely done with custom wrappers.

[mainameiz]

I think it would be nice if I could set attributes for specific elements

1. globally for all forms
2. globally for forms using a specific wrapper
3. inside a wrapper
   (with merged attributes' values from previous level)

What is the reason of adding wrap_with option? It could be accomplished with

element.wrapper tag: 'div', class: 'el-class' do |wrapper| ... end

isn't it?

[umhan35]

+1 for this

[stephenprater]

@rafaelfranca i hate to be an ass about this - but is there ANY CHANCE this could get merged? is there something i can do to help you?

[mchlfhr]

+1

Would lead to soo much cleaner code. I love to use different wrappers for different forms (especially for forms which have other dimensions in the Bootstrap 3 grid). But the only way to do so is currently (please correct me if I am wrong):

b.use :label , wrap_with: { tag: 'div', class: 'col-md-2 text-right' }
b.use :input , wrap_with: { tag: 'div', class: 'col-md-10' }

Shorter and no additional "div" around my label

b.use :label, :class => ['col-md-2']
b.use :input, :class => ['col-md-10']

[johnnyshields]

👍 +1

[rafaelfranca]

Yes, this will be merged. As you can see it is in the 3.1 milestone.

[rafaelfranca]

@stephenprater hey, I merged this PR in a separate branch and I found some problems.

The error classes and hint classes are getting duplicated. I think you had the same problem with input classes (this is why I think you called uniq there. Could you take a look?

[jrust]

👍 on this. I am currently using the rm-attributes-to-components branch to get bootstrap 3 horizontal forms working nicely. With this wrapper they work and are decently flexible:

  config.wrappers :horizontal, tag: :div, class: 'form-group', error_class: 'has-error' do |b|
    b.use :html5
    b.use :placeholder
    b.use :min_max
    b.use :maxlength

    b.optional :pattern
    b.optional :readonly

    b.use :label, class: 'col-md-2'
    b.wrapper :right_col, tag: :div, class: 'col-md-5' do |component|
      component.use :input
      component.use :hint,  wrap_with: { tag: 'p', class: 'help-block' }
      component.use :error, wrap_with: { tag: 'span', class: 'help-block has-error' }
    end
  end

The form column widths can then be overridden via the defaults in the simple_form tag:

simple_form_for @user, wrapper: :horizontal, html: { class: 'form-horizontal' }, defaults: { label_html: { class: 'col-md-3' }, right_col_html: { class: 'col-md-6' } } do |f|

Only caveat is that while the label_html col-md-4 default overrides the col-md-2 from the wrapper, the right_col_html default appends col-md-6 to the wrapper's col-md-5. Which means I can make right column bigger, but not smaller since md-6 overrides md-5. It would be nice if there were some way override rather than augment the right_col class, but for now at least its working!

[kjs3]

+1

[butsjoh]

We made some changes (younited/simple_form@ffecbe0) to have a consistent behaviour for the merging of html attributes. We made the following changes (happened on the 2.2 branch since we cannot move to 3 yet):

• We replaced the normal merge behaviour with deep_merge where applicable (eg: if f.input has defined a tabindex and and the wrapper adds a default class both are applied)
• We assumed that specifying the options on the input directly (using f.input) take precedence over the defaults configured on the form (defaults setting on form builder) and as a last resort it will use the ones defined on the wrappers.
• We assume that all the config options of the wrapper are used when defining a wrapper directly on an input

[Yankie]

+1

[jsmestad]

👍 this is extremely helpful when integrating simple_form with Foundation forms.