Allow procs for menu parent

[simonc]

Hi,

I got an error trying to pass a proc for menu :parent option. The error happens in Menu::MenuNode#normalize_id because Proc is not supported.

Is there a particular reason to disallow proc usage for parent ?

Thanks

[simonc]

Here is tiny patch that seems to be working. What do you think ?

[seanlinsley]

It's disallowed because it's difficult to coordinate between multiple children what the parent is. For example, would you want to copy & paste the proc between multiple AA registrations? Or have a global proc that can be accessed? What if the proc returns something different sometimes, so some children aren't property namespaced under the parent?

A better option is to define your complex menu structure in the initializer (or some other file) like described in the docs. That way this is possible:

ActiveAdmin.setup do |config|
  config.namespace :admin do |admin|
    admin.build_menu do |menu|
      menu.add label: ->{"Foo"} do |foo|
        foo.add label: "Bar", url: "http://github.com"
      end
    end
  end
end

[simonc]

Ok, if I understand correctly, if I have several resources to put under a parent menu it would like this ?

# app/admin/students.rb
ActiveAdmin.register Student do
  menu false
end

# app/admin/teachers.rb
ActiveAdmin.register Teacher do
  menu false
end

# config/initializers/active_admin.rb
ActiveAdmin.setup do |config|
  config.namespace :admin do |admin|
    admin.build_menu do |menu|
      menu.add label: ->{ I18n.t(:users) } do |m|
        m.add label: ->{ Student.model_name.human }, url: [:admin, :students]
        m.add label: ->{ Teacher.model_name.human }, url: [:admin, :teachers]
      end
    end
  end
end

[seanlinsley]

Yes, that's how it should work. Though to be honest, I haven't tried it myself.

[simonc]

I can tell you it works ;)

[simonc]

My bad, it doesn't. My patch was still active.
The following line is failing with the same error (Proc isn't supported as a Menu ID):

menu.add label: ->{ I18n.t(:users) } do |m|

And even when fixing this line the next one gets the error

m.add label: ->{ Student.model_name.human }, url: [:admin, :students]

Here it shouldn't be expected I think :/

[seanlinsley]

Oh okay, so I just dug up a conversation I had with @gregbell about this issue. I suggest you read it. For context, that discussion occured on my PR that would have let you pass a hash of options to your parent menu item, but was closed in favor of the syntax that lets you do the same in the initializer. The example @gregbell gave:

# config/initializers/active_admin.rb
# ...
config.namespace :admin do |admin|
  admin.build_menu do |menu|
    menu.add id: "foo", label: proc{ "Bar" }
  end
end

ActiveAdmin.register MyModel do
  menu parent: "foo" # will connect to the `foo` menu item
end

[simonc]

Ok I see. You create a menu entry, specifying the label so that menu items can connect to it later.
Indeed it seems cleaner and since the id is specified it should create the menu item without any id conversion issue :)

[simonc]

I just tested it and it seems to work fine. Is it documented any where outside the conversation ? Otherwise I'll add something to the wiki :)

[seanlinsley]

Yeah, this file should really mention the :id option and how it's useful for parents with a proc label: https://github.com/gregbell/active_admin/blob/master/docs/2-resource-customization.md

[seanlinsley]

BTW @simonc, I just came across this again and finally updated the docs: be7ba09

[simonc]

@seanlinsley awesome :)