Pass current index as arguments to inputs block for nested inputs #798

Merged
merged 2 commits into from Feb 14, 2012

Conversation

Projects
None yet
4 participants
@mhuggins
Contributor

mhuggins commented Feb 12, 2012

This is to fix issue #771. I previously submitted pull request #773, which had failing tests for older Rails versions. This not only addresses that, but is also a better solution I believe.

Now, when calling inputs on a form builder when passing a block, the second argument provided is the nested attribute index. e.g.:

<%= form.inputs :for => :tags do |tag, index| %>
  <%= tag.input :label => "Tag #{index}" %>
<% end %>

See the updated README in this pull request for another (better) example.

@justinfrench

This comment has been minimized.

Show comment Hide comment
@justinfrench

justinfrench Feb 12, 2012

Owner

Awesome work, and a nice solution I hadn't thought of! So by the looks of it, this is backwards compatible (you can ignore/omit the index from the block), and better/more flexible. We should probably deprecate the other method, but that can be a separate problem.

@sobrinho @haines @yabawock can you see any reason not to pull in?

Owner

justinfrench commented Feb 12, 2012

Awesome work, and a nice solution I hadn't thought of! So by the looks of it, this is backwards compatible (you can ignore/omit the index from the block), and better/more flexible. We should probably deprecate the other method, but that can be a separate problem.

@sobrinho @haines @yabawock can you see any reason not to pull in?

@mhuggins

This comment has been minimized.

Show comment Hide comment
@mhuggins

mhuggins Feb 12, 2012

Contributor

Thanks! And yes, it should be backwards compatible since it's simply adding a parameter that can be ignored (assuming there wasn't a different second parameter in the past that this would clash with).

The only reason I didn't try to deprecate the other method is because inputs doesn't always accept a block, meaning there is no way to utilize the index argument being passed to the block. For example, consider the following:

<%= f.inputs :name => 'Category #%i', :for => :categories %>
Contributor

mhuggins commented Feb 12, 2012

Thanks! And yes, it should be backwards compatible since it's simply adding a parameter that can be ignored (assuming there wasn't a different second parameter in the past that this would clash with).

The only reason I didn't try to deprecate the other method is because inputs doesn't always accept a block, meaning there is no way to utilize the index argument being passed to the block. For example, consider the following:

<%= f.inputs :name => 'Category #%i', :for => :categories %>
@mhuggins

This comment has been minimized.

Show comment Hide comment
@mhuggins

mhuggins Feb 13, 2012

Contributor

Shoot...I was updating my fork's branch to match what's currently on your master branch, and it ended up adding another commit. I don't think that should break anything since it's in master already, but if somehow does, let me know, and I'll see that it's undone.

Contributor

mhuggins commented Feb 13, 2012

Shoot...I was updating my fork's branch to match what's currently on your master branch, and it ended up adding another commit. I don't think that should break anything since it's in master already, but if somehow does, let me know, and I'll see that it's undone.

@mjonuschat

This comment has been minimized.

Show comment Hide comment
@mjonuschat

mjonuschat Feb 13, 2012

Collaborator

@justinfrench @mhuggins Looks great - no objections to merging this in asap from my side.

Collaborator

mjonuschat commented Feb 13, 2012

@justinfrench @mhuggins Looks great - no objections to merging this in asap from my side.

@sobrinho

This comment has been minimized.

Show comment Hide comment
@sobrinho

sobrinho Feb 13, 2012

Collaborator

@mhuggins nice job!

@justinfrench no objections too :)

Collaborator

sobrinho commented Feb 13, 2012

@mhuggins nice job!

@justinfrench no objections too :)

justinfrench added a commit that referenced this pull request Feb 14, 2012

Merge pull request #798 from mhuggins/indexed-labels
Pass current index as arguments to inputs block for nested inputs

@justinfrench justinfrench merged commit 82b552d into justinfrench:master Feb 14, 2012

@justinfrench

This comment has been minimized.

Show comment Hide comment
@justinfrench

justinfrench Feb 14, 2012

Owner

@mhuggins Boo build fail! http://travis-ci.org/#!/justinfrench/formtastic/builds/678637 Looks like REE and 1.8.7, can you have a quick look if there's any 1.9-isms that snuck in, or anything else you can do to help resolve?

Owner

justinfrench commented Feb 14, 2012

@mhuggins Boo build fail! http://travis-ci.org/#!/justinfrench/formtastic/builds/678637 Looks like REE and 1.8.7, can you have a quick look if there's any 1.9-isms that snuck in, or anything else you can do to help resolve?

@mhuggins

This comment has been minimized.

Show comment Hide comment
@mhuggins

mhuggins Feb 14, 2012

Contributor

Shoot. The tests are failing on REE & 1.8.7 wherever that second parameter is not included in the block. It looks like with the way I implemented this change, 1.8.x requires both parameters to be defined in the block, e.g.:

form.inputs do |builder, index|  # this is okay
    builder.input ...
end

form.inputs do |builder|  # this throws an error
    builder.input ...
end

I might not be skilled enough with Ruby to see a way around it other than explicitly looking into the number of args the proc expects, e.g.:

# change this in InputsHelper.inputs_for_nested_attributes...
index = parent_child_index(options[:parent]) if options[:parent]
block.call(f, index)

# to this...
if block.arity == 1
  block.call(f)
else
  index = parent_child_index(options[:parent]) if options[:parent]
  block.call(f, index)
end

Would that be acceptable, or does anyone have a better suggestion? Alternatively, is it acceptable to assume that nested attribute blocks should always accept two parameters from now on (if using 1.8.x), and should I just update the tests to account for the extra parameter? Thanks man!

Contributor

mhuggins commented Feb 14, 2012

Shoot. The tests are failing on REE & 1.8.7 wherever that second parameter is not included in the block. It looks like with the way I implemented this change, 1.8.x requires both parameters to be defined in the block, e.g.:

form.inputs do |builder, index|  # this is okay
    builder.input ...
end

form.inputs do |builder|  # this throws an error
    builder.input ...
end

I might not be skilled enough with Ruby to see a way around it other than explicitly looking into the number of args the proc expects, e.g.:

# change this in InputsHelper.inputs_for_nested_attributes...
index = parent_child_index(options[:parent]) if options[:parent]
block.call(f, index)

# to this...
if block.arity == 1
  block.call(f)
else
  index = parent_child_index(options[:parent]) if options[:parent]
  block.call(f, index)
end

Would that be acceptable, or does anyone have a better suggestion? Alternatively, is it acceptable to assume that nested attribute blocks should always accept two parameters from now on (if using 1.8.x), and should I just update the tests to account for the extra parameter? Thanks man!

@justinfrench

This comment has been minimized.

Show comment Hide comment
@justinfrench

justinfrench Feb 14, 2012

Owner

@mhuggins I don't want to change the APIs on a point release, so we can either revert, or try your work-around. Let's go with the latter!

Owner

justinfrench commented Feb 14, 2012

@mhuggins I don't want to change the APIs on a point release, so we can either revert, or try your work-around. Let's go with the latter!

@mhuggins

This comment has been minimized.

Show comment Hide comment
@mhuggins

mhuggins Feb 14, 2012

Contributor

Alright, I started coding on the bus ride home, but my laptop died...will finish and test as soon as I'm home. :)

Contributor

mhuggins commented Feb 14, 2012

Alright, I started coding on the bus ride home, but my laptop died...will finish and test as soon as I'm home. :)

@mhuggins

This comment has been minimized.

Show comment Hide comment
@mhuggins

mhuggins Feb 15, 2012

Contributor

Alright, check out pull request #800.

Contributor

mhuggins commented Feb 15, 2012

Alright, check out pull request #800.

justinfrench added a commit that referenced this pull request Feb 15, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment