add html classes to inputs with the wrappers API #622

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
@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

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jul 16, 2012

Collaborator

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']
Collaborator

rafaelfranca commented Jul 16, 2012

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

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jul 16, 2012

Collaborator

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

Collaborator

rafaelfranca commented Jul 16, 2012

@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

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jul 16, 2012

Collaborator

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

Collaborator

rafaelfranca commented Jul 16, 2012

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

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Jul 16, 2012

Member

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

Member

josevalim commented Jul 16, 2012

@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

This comment has been minimized.

Show comment Hide comment
@stephenprater

stephenprater Jul 16, 2012

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

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

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jul 16, 2012

Collaborator

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

Collaborator

carlosantoniodasilva commented Jul 16, 2012

@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

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jul 16, 2012

Collaborator

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.

Collaborator

rafaelfranca commented Jul 16, 2012

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

This comment has been minimized.

Show comment Hide comment
@stephenprater

stephenprater Jul 16, 2012

@rafaelfranca gotcha. makes sense now.

@rafaelfranca gotcha. makes sense now.

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jul 16, 2012

Collaborator

@carlosantoniodasilva yes, we can leave the SimpleForm configuration.

Collaborator

rafaelfranca commented Jul 16, 2012

@carlosantoniodasilva yes, we can leave the SimpleForm configuration.

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jul 16, 2012

Collaborator

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

Collaborator

rafaelfranca commented Jul 16, 2012

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

@stephenprater

This comment has been minimized.

Show comment Hide comment
@stephenprater

stephenprater Jul 16, 2012

@rafaelfranca yes, i'll work on it.

@rafaelfranca yes, i'll work on it.

@stephenprater

This comment has been minimized.

Show comment Hide comment
@stephenprater

stephenprater Jul 17, 2012

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

@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

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jul 17, 2012

Collaborator

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

Collaborator

rafaelfranca commented Jul 17, 2012

@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

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jul 17, 2012

Collaborator
Collaborator

rafaelfranca commented Jul 17, 2012

lib/simple_form/wrappers/single.rb
super(name, [name], options)
end
def render(input)
options = input.options
if options[namespace] != false
+ if [:input, :label_input].include? namespace

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jul 17, 2012

Collaborator

why :input is getting a different approach?

@rafaelfranca

rafaelfranca Jul 17, 2012

Collaborator

why :input is getting a different approach?

This comment has been minimized.

Show comment Hide comment
@stephenprater

stephenprater Jul 17, 2012

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.

@stephenprater

stephenprater Jul 17, 2012

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.

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jul 18, 2012

Collaborator

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

@rafaelfranca

rafaelfranca Jul 18, 2012

Collaborator

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

lib/simple_form/wrappers/single.rb
super(name, [name], options)
end
def render(input)
options = input.options
if options[namespace] != false
+ if [:input, :label_input].include? namespace
+ input.input_html_classes.concat Array.wrap(wrapper_html_options[:class])

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

no need to use Array.wrap (I mean, Array() only should be enough, right?)

@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

no need to use Array.wrap (I mean, Array() only should be enough, right?)

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jul 18, 2012

Collaborator

I think we need. Kernel#Array doesn't play nice in Ruby 1.8

@rafaelfranca

rafaelfranca Jul 18, 2012

Collaborator

I think we need. Kernel#Array doesn't play nice in Ruby 1.8

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

Ow 💩, I forgot we support 1.8 here 😄.

Anyway, what should this :class option support? nil, [], and maybe 'foobar'? Those should be good I think?!?

>> Array(nil)
=> []
>> Array([])
=> []
>> Array('foo bar')
=> ["foo bar"]

Well, in any case @stephenprater, just ignore the Array.wrap comments, and let them out there, we probably have it in other places too :)

@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

Ow 💩, I forgot we support 1.8 here 😄.

Anyway, what should this :class option support? nil, [], and maybe 'foobar'? Those should be good I think?!?

>> Array(nil)
=> []
>> Array([])
=> []
>> Array('foo bar')
=> ["foo bar"]

Well, in any case @stephenprater, just ignore the Array.wrap comments, and let them out there, we probably have it in other places too :)

lib/simple_form/wrappers/single.rb
+ (input.options["#{local_namespace}_html".intern] ||= {}).tap do |o|
+ if o[:class] or wrapper_html_options[:class]
+ o[:class] = Array.wrap(o[:class])
+ o[:class].concat Array.wrap(wrapper_html_options[:class])

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

Ditto

lib/simple_form/wrappers/single.rb
- [:label, :input].include?(namespace) ? {} : super
+ def html_options_for_input(input, local_namespace = namespace)
+ (input.options["#{local_namespace}_html".intern] ||= {}).tap do |o|
+ if o[:class] or wrapper_html_options[:class]

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

|| instead of or

@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

|| instead of or

test/form_builder/wrapper_test.rb
+ end
+
+ test 'single elment with wrap with and other options does both' do
+ swap_wrapper :default, self.custom_wrapper_with_no_wrapping_tag_and_wrapping_tag do

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

there's no need for self, here in the other above.

@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

there's no need for self, here in the other above.

test/form_builder/wrapper_test.rb
+ end
+ end
+
+

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

two whitespaces ✂️

@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

two whitespaces ✂️

test/form_builder/wrapper_test.rb
+ end
+ end
+
+ test 'single elment with wrap with and other options does both' do

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

typo element

@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

typo element

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

does both what? it's not clear to me what this test actually does :)

@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

does both what? it's not clear to me what this test actually does :)

test/support/misc_helpers.rb
+ end
+ end
+
+ def custom_wrapper_with_no_wrapping_tag_and_wrapping_tag

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

This name looks a little bit weird: with_no_wrapping_tag_and_wrapping_tag?

@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

This name looks a little bit weird: with_no_wrapping_tag_and_wrapping_tag?

lib/simple_form/wrappers/single.rb
- def html_options(options)
- [:label, :input].include?(namespace) ? {} : super
+ def html_options_for_input(input, local_namespace = namespace)
+ (input.options["#{local_namespace}_html".intern] ||= {}).tap do |o|

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

There's no apparent need for tap here, since the result of this method is not used anywhere (the goal is to merge/concat stuff). I think it could get a bit clearer by using a simple variable instead of tap + block.

@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

There's no apparent need for tap here, since the result of this method is not used anywhere (the goal is to merge/concat stuff). I think it could get a bit clearer by using a simple variable instead of tap + block.

@stephenprater

This comment has been minimized.

Show comment Hide comment
@stephenprater

stephenprater Jul 31, 2012

Anything more I need to do to get this merged?

Anything more I need to do to get this merged?

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jul 31, 2012

Collaborator

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

Collaborator

rafaelfranca commented Jul 31, 2012

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

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Aug 25, 2012

Collaborator

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

Collaborator

rafaelfranca commented Aug 25, 2012

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

lib/simple_form/wrappers/builder.rb
@@ -40,6 +40,11 @@ module Wrappers
# In the example above, hint defaults to false, which means it won't automatically
# do the lookup anymore. It will only be triggered when :hint is explicitly set.
class Builder
+
+ class_attribute :non_tag_components
+ self.non_tag_components = [:placeholder, :error, :hint, :html5,

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Aug 25, 2012

Collaborator

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

@rafaelfranca

rafaelfranca Aug 25, 2012

Collaborator

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

This comment has been minimized.

Show comment Hide comment
@stephenprater

stephenprater Aug 27, 2012

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.

@stephenprater

stephenprater Aug 27, 2012

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.

lib/simple_form/wrappers/builder.rb
- options = { :wrap_with => options }
+ if options && (self.class.non_tag_components.include?(name) \
+ && !(options.except(:wrap_with).keys.empty?))
+ warn "Invalid options #{options.except(:wrap_with).keys.inspect} passed to #{name}."

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Aug 25, 2012

Collaborator

We not use the ActiveSupport::Deprecation.warn?

@rafaelfranca

rafaelfranca Aug 25, 2012

Collaborator

We not use the ActiveSupport::Deprecation.warn?

This comment has been minimized.

Show comment Hide comment
@stephenprater

stephenprater Aug 27, 2012

It's not a depreciation warning so much as a "you're using this wrong" warning - I wouldn't want this quashed by ActiveSupport::Depreciation behaviors that ignore depreciation warnings, on the other hand the more that I think about this - the more that I think it should probably just raise an exception. i can't think of any reason why you would ever want to do this.

@stephenprater

stephenprater Aug 27, 2012

It's not a depreciation warning so much as a "you're using this wrong" warning - I wouldn't want this quashed by ActiveSupport::Depreciation behaviors that ignore depreciation warnings, on the other hand the more that I think about this - the more that I think it should probably just raise an exception. i can't think of any reason why you would ever want to do this.

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Aug 27, 2012

Collaborator

Yes, raise an exception seems better.

@rafaelfranca

rafaelfranca Aug 27, 2012

Collaborator

Yes, raise an exception seems better.

lib/simple_form/wrappers/builder.rb
- "Please invoke b.use #{name.inspect}, :wrap_with => #{options.inspect} instead."
- options = { :wrap_with => options }
+ if options && (self.class.non_tag_components.include?(name) \
+ && !(options.except(:wrap_with).keys.empty?))

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Aug 25, 2012

Collaborator

We can still use the options.keys != [:wrap_with] check here.

@rafaelfranca

rafaelfranca Aug 25, 2012

Collaborator

We can still use the options.keys != [:wrap_with] check here.

lib/simple_form/wrappers/single.rb
@@ -2,13 +2,24 @@ module SimpleForm
module Wrappers
# `Single` is an optimization for a wrapper that has only one component.
class Single < Many
+ attr_reader :wrapper_html_options
+
def initialize(name, options={})

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Aug 25, 2012

Collaborator
def initialize(name, options={})
  @wrapper_html_options = options.except(:wrap_with)
  super(name, [name], options.fetch(:wrap_with, {}))
end
@rafaelfranca

rafaelfranca Aug 25, 2012

Collaborator
def initialize(name, options={})
  @wrapper_html_options = options.except(:wrap_with)
  super(name, [name], options.fetch(:wrap_with, {}))
end
lib/simple_form/wrappers/single.rb
@@ -16,6 +27,15 @@ def render(input)
private
+ def html_options_for_input(input, local_namespace = namespace)
+ options = ( input.options["#{local_namespace}_html".intern] ||= {} )

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Aug 25, 2012

Collaborator

use to_sym here

@rafaelfranca

rafaelfranca Aug 25, 2012

Collaborator

use to_sym here

lib/simple_form/wrappers/single.rb
@@ -16,6 +27,15 @@ def render(input)
private
+ def html_options_for_input(input, local_namespace = namespace)

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Aug 25, 2012

Collaborator

This method is too confusing. What is the intent?

@rafaelfranca

rafaelfranca Aug 25, 2012

Collaborator

This method is too confusing. What is the intent?

This comment has been minimized.

Show comment Hide comment
@stephenprater

stephenprater Aug 27, 2012

this merges all the different places that you could set classes with either more or less granularity into the particular input instance we are rendering, and sets up the options hash in the format expected by SimpleForm::Wrappers::Many#wrap

i'll add some comments to that affect.

@stephenprater

stephenprater Aug 27, 2012

this merges all the different places that you could set classes with either more or less granularity into the particular input instance we are rendering, and sets up the options hash in the format expected by SimpleForm::Wrappers::Many#wrap

i'll add some comments to that affect.

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Aug 25, 2012

Collaborator

Sorry for the delay. I made some comments in the diff

Collaborator

rafaelfranca commented Aug 25, 2012

Sorry for the delay. I made some comments in the diff

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Aug 27, 2012

This pull request passes (merged 57efd092 into 6d4af55).

This pull request passes (merged 57efd092 into 6d4af55).

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Aug 27, 2012

This pull request passes (merged 8957072 into 46d4bb4).

This pull request passes (merged 8957072 into 46d4bb4).

lib/simple_form/wrappers/single.rb
end
def render(input)
options = input.options
if options[namespace] != false
+ #inputs store there html classes in a separate place from other tags
+ #or non-tag components - premerge them before collecting alll
+ #possible html_options

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Sep 8, 2012

Collaborator

Please fix these comments, add spaces after #, remove there, alll is wrong, finish the sentence with a .. 2 lines should be enough, thanks.

@carlosantoniodasilva

carlosantoniodasilva Sep 8, 2012

Collaborator

Please fix these comments, add spaces after #, remove there, alll is wrong, finish the sentence with a .. 2 lines should be enough, thanks.

lib/simple_form/wrappers/single.rb
+ # the wrappers api, etc) we merge them all together here so that that
+ # SimpleForm::Wrappers::Many#wrap know what do with them, taking care
+ # to leave options[:class] as an array if necessary
+ options = ( input.options["#{local_namespace}_html".to_sym] ||= {} )

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Sep 8, 2012

Collaborator

Don't use spaces with ().

@carlosantoniodasilva

carlosantoniodasilva Sep 8, 2012

Collaborator

Don't use spaces with ().

test/form_builder/wrapper_test.rb
+ with_concat_form_for @user do |f|
+ concat f.input :name, :invalid => 'thing'
+ end
+ assert_no_select "input[invalid='thing']"

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Sep 8, 2012

Collaborator

You can just check for input[invalid], no need for thing

@carlosantoniodasilva

carlosantoniodasilva Sep 8, 2012

Collaborator

You can just check for input[invalid], no need for thing

+ assert_select "div.custom_wrapper div.elem label[data-yo='yo']"
+ assert_select "div.custom_wrapper div.elem span.custom_yo", :text => "custom"
+ assert_select "div.custom_wrapper div.elem label.both_yo"
+ assert_select "div.custom_wrapper div.elem input.both_yo"

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Sep 8, 2012

Collaborator

assert_select accepts a block, may make this a bit more clear?

@carlosantoniodasilva

carlosantoniodasilva Sep 8, 2012

Collaborator

assert_select accepts a block, may make this a bit more clear?

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Sep 8, 2012

Collaborator

Well, forget about it. Other tests need more clean up with that, we can do that later.

@carlosantoniodasilva

carlosantoniodasilva Sep 8, 2012

Collaborator

Well, forget about it. Other tests need more clean up with that, we can do that later.

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Sep 8, 2012

Collaborator

@stephenprater thank you for your work on that.

@rafaelfranca I'm ok with the feature, it is definitely useful. Feel free to do the changes you think that are required, I have made a few comments, but we'll need to review and give some more thought to the entire wrappers api afterwards.

Just want to note that I'd rather merge this after a new release.

Thanks guys! <3

Collaborator

carlosantoniodasilva commented Sep 8, 2012

@stephenprater thank you for your work on that.

@rafaelfranca I'm ok with the feature, it is definitely useful. Feel free to do the changes you think that are required, I have made a few comments, but we'll need to review and give some more thought to the entire wrappers api afterwards.

Just want to note that I'd rather merge this after a new release.

Thanks guys! <3

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva May 20, 2013

Collaborator

@why-el thanks, we'll be handling this after we get 3.0 final out. I don't want to introduce big changes and new features right now, but it'll be part of Simple Form in the near future.

Collaborator

carlosantoniodasilva commented May 20, 2013

@why-el thanks, we'll be handling this after we get 3.0 final out. I don't want to introduce big changes and new features right now, but it'll be part of Simple Form in the near future.

@mensfeld

This comment has been minimized.

Show comment Hide comment
@mensfeld

mensfeld May 25, 2013

still waiting guys ;)

still waiting guys ;)

@jcoleman

This comment has been minimized.

Show comment Hide comment
@jcoleman

jcoleman Jun 22, 2013

Any projected timeline on this?

Any projected timeline on this?

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jun 22, 2013

Collaborator

No. We will do when we can. We have a lot of things to do and after all this is open source, we are not being paid to work only on this project.

But be sure we want this in too. It is not the top priority to us. After the Rails 4 release and the Simple Form 3 we will revisit this like we said above.

Thanks.

Collaborator

rafaelfranca commented Jun 22, 2013

No. We will do when we can. We have a lot of things to do and after all this is open source, we are not being paid to work only on this project.

But be sure we want this in too. It is not the top priority to us. After the Rails 4 release and the Simple Form 3 we will revisit this like we said above.

Thanks.

@gmile

This comment has been minimized.

Show comment Hide comment
@gmile

gmile Jul 27, 2013

Guys, now that Bootstrap 3.0 RC1 is out, having this PR merged would be super useful. For the bootstrap form styling to propertly take place, it is now required that all inputs (including <textarea>, <select> etc.) have a .form-control class, and all labels to have .control-label class.

So far I see 2 options one could go with to workaroun current's SimpleForm limit:

  1. create custom replacements for all kinds of <input> tags (including <textarea>, <select> etc.) with the .form-control class set (as well as subclass default label to always include .control-label class),

  2. set desired .form-control ad-hoc for any input that requires it, as such (same for ):

    = f.input :title, input_html: { class: 'form-control' }

Yet a better option here would be exactly what this PR is suggesting – globally set classes on all inputs and labels inside a particular wrapper definition.

gmile commented Jul 27, 2013

Guys, now that Bootstrap 3.0 RC1 is out, having this PR merged would be super useful. For the bootstrap form styling to propertly take place, it is now required that all inputs (including <textarea>, <select> etc.) have a .form-control class, and all labels to have .control-label class.

So far I see 2 options one could go with to workaroun current's SimpleForm limit:

  1. create custom replacements for all kinds of <input> tags (including <textarea>, <select> etc.) with the .form-control class set (as well as subclass default label to always include .control-label class),

  2. set desired .form-control ad-hoc for any input that requires it, as such (same for ):

    = f.input :title, input_html: { class: 'form-control' }

Yet a better option here would be exactly what this PR is suggesting – globally set classes on all inputs and labels inside a particular wrapper definition.

@stephenprater

This comment has been minimized.

Show comment Hide comment
@stephenprater

stephenprater Jul 30, 2013

rebased against current master - no more complicated merging and rebasing, this pull request she will now apply cleanly.

rebased against current master - no more complicated merging and rebasing, this pull request she will now apply cleanly.

@sfaxon

This comment has been minimized.

Show comment Hide comment
@sfaxon

sfaxon Jul 30, 2013

+1 @stephenprater's rebased patch worked for me on a rails 4 app.

sfaxon commented Jul 30, 2013

+1 @stephenprater's rebased patch worked for me on a rails 4 app.

@TALlama

This comment has been minimized.

Show comment Hide comment
@TALlama

TALlama Jul 31, 2013

@gmile another option, if you're using SASS or LESS for styling, is to add a form-control-wrapper class to the input's wrapper with b.use :input, wrap_with: { class: 'form-control-wrapper' }, and then pull in the .form-control styles from the Bootstrap CSS files:

.form-control-wrapper {
  select, 
  textarea, 
  input[type="text"], 
  input[type="password"], 
  input[type="datetime"], 
  input[type="datetime-local"], 
  input[type="date"], 
  input[type="month"], 
  input[type="time"], 
  input[type="week"], 
  input[type="number"], 
  input[type="email"], 
  input[type="url"], 
  input[type="search"], 
  input[type="tel"], 
  input[type="color"] {
    .form-control /* for LESS */
    @extend .form-control /* for SASS */
  }
}

This is, to be very clear, hackish and brittle, and getting this PR merged would be better, but it allows you to use Bootstrap 3 now.

TALlama commented Jul 31, 2013

@gmile another option, if you're using SASS or LESS for styling, is to add a form-control-wrapper class to the input's wrapper with b.use :input, wrap_with: { class: 'form-control-wrapper' }, and then pull in the .form-control styles from the Bootstrap CSS files:

.form-control-wrapper {
  select, 
  textarea, 
  input[type="text"], 
  input[type="password"], 
  input[type="datetime"], 
  input[type="datetime-local"], 
  input[type="date"], 
  input[type="month"], 
  input[type="time"], 
  input[type="week"], 
  input[type="number"], 
  input[type="email"], 
  input[type="url"], 
  input[type="search"], 
  input[type="tel"], 
  input[type="color"] {
    .form-control /* for LESS */
    @extend .form-control /* for SASS */
  }
}

This is, to be very clear, hackish and brittle, and getting this PR merged would be better, but it allows you to use Bootstrap 3 now.

@stephenprater

This comment has been minimized.

Show comment Hide comment
@stephenprater

stephenprater Aug 4, 2013

@gmile - I'm using Bootstrap for this as well - so if you run into any problems let me know so I can update the PR.

@gmile - I'm using Bootstrap for this as well - so if you run into any problems let me know so I can update the PR.

@guigs

This comment has been minimized.

Show comment Hide comment
@guigs

guigs Aug 9, 2013

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' }?

guigs commented Aug 9, 2013

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' }?

@carlosantoniodasilva carlosantoniodasilva referenced this pull request in rafaelfranca/simple_form-bootstrap Aug 23, 2013

Closed

Support Bootstrap 3 #26

@jrhorn424

This comment has been minimized.

Show comment Hide comment
@jrhorn424

jrhorn424 Sep 9, 2013

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

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

This comment has been minimized.

Show comment Hide comment
@stephenprater

stephenprater Sep 10, 2013

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)

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

This comment has been minimized.

Show comment Hide comment
@jrhorn424

jrhorn424 Sep 10, 2013

Thanks, @stephenprater.

@ZenCocoon

This comment has been minimized.

Show comment Hide comment
@ZenCocoon

ZenCocoon Nov 1, 2013

Contributor

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.

Contributor

ZenCocoon commented Nov 1, 2013

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

This comment has been minimized.

Show comment Hide comment
@mainameiz

mainameiz Nov 4, 2013

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?

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

This comment has been minimized.

Show comment Hide comment
@umhan35

umhan35 Nov 6, 2013

+1 for this

umhan35 commented Nov 6, 2013

+1 for this

Stephen Prater and others added some commits Jul 12, 2012

add html classes to inputs with the wrappers API
enable a user to add classes or other html attributes to inputs,
labels, and custome components using the wrappers API.

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

A custom component which wants to support this functionality
should look in `options[:#{namespace}_html]` for the html
options to use.

An ArgumentError is raised if an html attribue is passed to any
component in `SimpleForm::FormBuilder::ATTRIBUTE_COMPONENTS` but
otherwise no validation is done - so it's possible to pass invalid
attributes to any tag generating component.
fix the use case of switching wrappers on a per input basis
  modified a test to cover this case and changed it to no longer
  memoize the input / label_html options hash. There's no big performance
  hit to doing so and it keeps the per input wrapper switching
  implementation clean
Actually TEST the behavior thought to to be tested in the #c02f95
  also, fix the behavior by making sure that wrapper options passed to a
  particular input override the options passed to the original builder in
  `SimpleForm::Inputs::Base#html_options_for` - this might be an existing
  bug exposed by this test since changing the code in this manner causes
  no test failures - and at least the behavior is now codified in a test
@stephenprater

This comment has been minimized.

Show comment Hide comment
@stephenprater

stephenprater Nov 9, 2013

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

@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

This comment has been minimized.

Show comment Hide comment
@mchlfhr

mchlfhr Nov 9, 2013

+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']

mchlfhr commented Nov 9, 2013

+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

This comment has been minimized.

Show comment Hide comment
@johnnyshields

johnnyshields Nov 9, 2013

👍 +1

👍 +1

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Nov 9, 2013

Collaborator

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

Collaborator

rafaelfranca commented Nov 9, 2013

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

@ghost ghost assigned rafaelfranca Nov 12, 2013

rafaelfranca added a commit that referenced this pull request Nov 12, 2013

Merge pull request #622 from stephenprater/classes_on_use
add html classes to inputs with the wrappers API
@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Nov 13, 2013

Collaborator

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

Collaborator

rafaelfranca commented Nov 13, 2013

@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

This comment has been minimized.

Show comment Hide comment
@jrust

jrust Nov 20, 2013

👍 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!

jrust commented Nov 20, 2013

👍 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

This comment has been minimized.

Show comment Hide comment
@kjs3

kjs3 Nov 26, 2013

+1

kjs3 commented Nov 26, 2013

+1

butsjoh added a commit to younited/simple_form that referenced this pull request Nov 28, 2013

Merge pull request #622 from stephenprater/classes_on_use
add html classes to inputs with the wrappers API

Conflicts:
	lib/simple_form/wrappers/builder.rb
	test/support/misc_helpers.rb
@butsjoh

This comment has been minimized.

Show comment Hide comment
@butsjoh

butsjoh Nov 28, 2013

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

butsjoh commented Nov 28, 2013

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

This comment has been minimized.

Show comment Hide comment
@Yankie

Yankie Dec 15, 2013

+1

Yankie commented Dec 15, 2013

+1

@jsmestad

This comment has been minimized.

Show comment Hide comment
@jsmestad

jsmestad Dec 31, 2013

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

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

@rafaelfranca rafaelfranca referenced this pull request in rafaelfranca/simple_form-bootstrap Jan 16, 2014

Merged

Bootstrap 3 support #28

11 of 11 tasks complete

@rafaelfranca rafaelfranca closed this in #997 Mar 17, 2014

mru2 added a commit to jobteaser/simple_form that referenced this pull request May 25, 2015

@ghost ghost referenced this pull request in jobteaser/simple_form May 25, 2015

Merged

Backport pull request #622 to simple_form 2.1.x #1

pinouchon pushed a commit to jobteaser/simple_form that referenced this pull request May 25, 2015

Benjamin Crouzier
Merge pull request #1 from jobteaser/allow-input-class-in-wrapper
Backport pull request #622 to simple_form 2.1.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment