Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support id_column and index_column in table_for #6461

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

c960657
Copy link

@c960657 c960657 commented Sep 19, 2020

id_column and index_column are not only useful in index tables but also in table_for. Move these two methods to the parent case, so they are available for any table.

I renamed the i18n option for table_for to resource_class (with fallback to i18n for BC), because with this change, the specified model is used not only for i18n purposes but also for determining the primary key.

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just made a few comments, but this PR looks very good!

docs/12-arbre-components.md Outdated Show resolved Hide resolved
@@ -27,7 +27,7 @@ Feature: Belongs To
"""
ActiveAdmin.register User
ActiveAdmin.register Post do
belongs_to :user
belongs_to :author, class_name: "User", param: "user_id", route_name: "user"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these and the other changes in this file related to this PR?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they are needed. I am not too familiar with belongs_to, but AFAICT the arguments are necessary for this to work at all (because the class name, User, does not match the attribute name, author). But I don't understand why the tests did not fail before. I'll try to figure out what is going on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you figure it out @c960657?

lib/active_admin/views/components/table_for.rb Outdated Show resolved Hide resolved
lib/active_admin/views/index_as_table.rb Show resolved Hide resolved
spec/unit/views/components/table_for_spec.rb Outdated Show resolved Hide resolved
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a very small grammar comment, and also curious about why specs didn't fail before without the belongs_to options.

If you can also rebase while fixing the grammar nit, that'd be great.

And sorry for the long delay 🙏.

The `column` method can take a title as its first argument and data
(`:your_method`) as its second (or first if no title provided). Column also
takes a block.
`column`, `index_column`, and `id_column` works like for [index tables](3-index-pages/index-as-table.md).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`column`, `index_column`, and `id_column` works like for [index tables](3-index-pages/index-as-table.md).
`column`, `index_column`, and `id_column` work like for [index tables](3-index-pages/index-as-table.md).

I think "work" is grammatically correct?

@c960657
Copy link
Author

c960657 commented Feb 1, 2021

And sorry for the long delay 🙏.

Likewise :-)

Just a very small grammar comment, and also curious about why specs didn't fail before without the belongs_to options.

The difference is caused by the change from resource_path to config.route_instance_path. The former is somehow able to extract the User from the Post parameter even without the belongs_to parameters, but the latter isn't. There is a lot of magic going on here, and I cannot really grasp it.

@kwent
Copy link

kwent commented Jun 2, 2022

We also need index_column

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants