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

Index page title can be set using a proc #1861

Merged
merged 3 commits into from Mar 4, 2013

Conversation

jamesalmond
Copy link
Contributor

I've implemented functionality that allows you to set dynamic titles on index actions, which should resolve #1324 and #1059 (closed due to inactivity?). The proc is instance eval-ed against the controller as there isn't a resource to apply it to in the index action.

I implemented it because needed to modify titles for belongs_to resources to reference their parent. For example, if an Article belongs to a Brand the article index action would be:

index title: proc{ "#{@brand.name}: Articles" } do
  # ...
end

@seanlinsley
Copy link
Contributor

Hi @jamesalmond, owner of #1324 here. Glad to see you added this. It was originally included it in my PR, but I couldn't imagine a use-case for procs at the time.

@@ -10,3 +10,12 @@ Feature: Index - Page Title
end
"""
Then I should see the page title "Awesome Title"

Scenario: Set a string as the title
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this say something like "Use a proc for the title"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, my bad! Pushed a corrected title.

@jamesalmond
Copy link
Contributor Author

Hi @daxter. Yes, I saw you'd added it (and then removed it!) :) Coming across a few limitations to what I want to achieve with AA so glad to have the time to try and resolve some of them.

I think it makes sense to have it in the case for nested resources, just not sure about the the context of the instance_eval. Any thoughts?

@seanlinsley
Copy link
Contributor

AFAICT, controller.instance_eval seems to be the correct context. Though you might want to add a test that captures the context with a faux resource, to ensure that this continues working in the future.

@jamesalmond
Copy link
Contributor Author

@daxter I'm not really sure what you mean by "capture the context with a faux resource". As far as I could tell there was no resource to access in this context.

@seanlinsley
Copy link
Contributor

Isn't there? In the example that you gave, what variable are you accessing?

On Jan 16, 2013, at 10:26 AM, James Almond notifications@github.com wrote:

@daxter I'm not really sure what you mean by "capture the context with a faux resource". As far as I could tell there was no resource to access in this context.


Reply to this email directly or view it on GitHub.

@jamesalmond
Copy link
Contributor Author

I'm accessing the parent in the belongs_to relationship. I believe inherited_resources sets it when you use belongs_to. I ended up instance_eval-ing against the controller as the way you'd written it in #1324 didn't appear to work any more.

@seanlinsley
Copy link
Contributor

You're right, the version in my PR didn't quite work (because I wasn't sure in what way it would be used).

Are you sure you want to instance_eval the controller? Wouldn't this be more appropriate?

resource_class.instance_eval(&config[:title])

That way you have direct access to the resource:

index :title => proc{"#{rand} #{self}"}

@jamesalmond
Copy link
Contributor Author

For my use case, I don't think accessing the resource class would work. That would be using instance_eval against the Article class and wouldn't give me access to the brand that's instantiated by inherited_resources which is what I need to include in my title. So

index :title => proc{"#{rand} #{self}"}

would be the equivalent of doing

index :title => proc{"#{Article.brand} #{Article}"}

which wouldn't give you to access to any of the request's context. Something about using instance_eval against the controller bugs me but I'm not sure what else would be suitable whilst being able to achieve the custom titles I need. Unless I'm not understanding what resource_class actually is at this point?

@seanlinsley
Copy link
Contributor

No you understood correctly (except I was calling rand to provide a random number, proving that the proc was re-evaluated).

BTW, with the version you're currently using, it would be more DRY to use resource_class instead of directly referencing Article.

In that case, you have the best implementation. Though you should add tests to confirm that the proc has access to the controller and resource_class. (that's what I meant by "context" yesterday)

@jamesalmond
Copy link
Contributor Author

I've added an example that uses resource_class in the proc title. You're right about the DRY-ness, I was just using the class explicitly in my example to show that using instance eval against it wouldn't give you access to the controller.

@seanlinsley
Copy link
Contributor

✅ looks good to me!

@mspanc
Copy link
Contributor

mspanc commented Feb 7, 2013

I would be glad to see this included in AA

@ekortright
Copy link

I would also be very happy to see this in ActiveAdmin. I just upgraded from 0.4.4 because I thought this was already available in 0.5.1 (the examples given here might warn unsuspecting users that procs and methods don't apply everywhere!).

@seanlinsley
Copy link
Contributor

@ekortright sorry about that! Besides procs on index pages, are there any other places you've noticed this problem?

@ekortright
Copy link

No problem! That was the only place I noticed it.

@macfanatic
Copy link
Contributor

Looks good to me, going to merge in.

macfanatic added a commit that referenced this pull request Mar 4, 2013
Index page title can be set using a proc
@macfanatic macfanatic merged commit fb9b14d into activeadmin:master Mar 4, 2013
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

5 participants