Add multi-record support to attributes_table_for

[zorab47]

Allow multiple records to be displayed in columns using the ActiveAdmin attribute_table_for component, while staying backward compatible with the previous behavior. It also builds a colgroup and col elements to facilitate styling each record's table column, but only when a collection is provided.

The PR is a result of discussions in #2540.

[coveralls]

Coverage decreased (-0.17%) when pulling 406d7bd on zorab47:multi-record-attributes-table-for into 1f4f90c on gregbell:master.

[seanlinsley]

💙

But do you really think attributes_table_for is a descriptive name of this functionality? I'm in favor of renaming it and deprecating the current method.

[zorab47]

Were you thinking of something like this?

table_for collection, by: :row do
  field :id
  field :name
end

or maybe this?

table_for collection, display: :attributes do; end
table_for collection, display: :columns do; end
table_for collection, display: :rows do; end # <= the default behavior

What would be the best approach for moving to a new API like this? Provide deprecated aliases for the existing API?

[seanlinsley]

What would display: :attributes do?

I think I'm in favor of display: :rows and display: :columns, but I'll need to sleep on it.

Yeah, the idea is we'd keep aliases for the existing API for at least the next release.

If we're going to merge these, I'm still conflicted on the field vs column & row question...

[zorab47]

I like to try all possible names to see which would be the best. Display :attributes could be an "alias" for the attributes_table_for, but it would probably be best to drop it entirely.

I think this new API would take the form:

table_for collection do
  column :name
end

table_for collection, display: :columns do
  row :name
end

The calls row and column would be aliases, which helps convey what type table is being constructed. In reality, they are defining an attribute field for the table. And to be the most flexible add another alias as field.

There is still potential for confusion. If a user wants to change the table format to be displayed by rows; she converts all the column calls to row, but then forgets to set the :display option. This may lead to several questions.

[seanlinsley]

Maybe if you're using row or column in the wrong table, we could raise an exception.

On Oct 8, 2013, at 8:12 AM, Charles Maresh [email protected] wrote:

I like to try all possible names to see which would be the best. Display :attributes could be an "alias" for the attributes_table_for, but it would probably be best to drop it entirely.

I think this new API would take the form:

table_for collection do
column :name
end

table_for collection, display: :rows do
row :name
end
The calls row and column would be aliases, which helps convey what type table is being constructed. In reality, they are defining an attribute field for the table. And to be the most flexible add another alias as field.

There is still potential for confusion. If a user wants to change the table format to be displayed by rows; she converts all the column calls to row, but then forgets to set the :display option. This may lead to several questions.

�
Reply to this email directly or view it on GitHub.

[zorab47]

A more intuitive solution would be to display the table by columns if all the calls are row and display the table by rows if all the calls are column. Then an error must be raised if there is a mixture of the two calls.

[shekibobo]

I think I'm with @zorab47 on this one.

table_for collection do
  row :id
  row :name
  row :created_at
end

table_for collection do
  column :id
  column :name
  column :created_at
end

Feels much more intuitive. In fact I'm pretty sure I've absent-mindedly tried to do this on multiple occasions.

I'd also vote for columns and rows support, as well, for passing multiple fields in one call.

[seanlinsley]

I'm not sure how we'll deal with the built in selectable_column, id_column and actions, but I like the idea.

It should probably be its own PR though, after this one. Do you agree @zorab47? If so, I'll merge this one.

[zorab47]

@daxter I'm good with that approach.

[shekibobo]

TIL id_column

[seanlinsley]

Yeah, I don't think id_column is documented anywhere. I found it when spelunking through the code.

[seanlinsley]

Thanks for the contribution! 🐼

[seanlinsley]

@zorab47 are you planning on implementing the new table_for? If not I'm more than happy to.

[zorab47]

@daxter it is your call. I won't have time to dedicate to it until this coming weekend.