[1283] Parent Menu Items #1488

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
6 participants
@seanlinsley
Owner

seanlinsley commented Jul 9, 2012

For #1283
With these changes you can pass settings to parent to customize your parent menu, which is especially useful when you have a catch-all parent menu that doesn't have its own page (so you can't configure it there).

menu :label => 'Users', :priority => 0, :parent => {
  :label => 'Administration',      :priority=> 3,
  :url => 'that_place/over_there', :id => '...?' 
}

UPDATES

  • adoped @gregbell's proposed syntax, allowing :parent to be a string, proc, or hash of options
  • my first commit broke the (already broken) parent label proc functionality from #1664 (only evaluates once)
    • my second commit added it back in, this time in a way that the proc gets re-evaluated on page load

TODO

  • write proper documentation, including:
    • @rainchen's solution for parent menus as an alternative
    • explanation that id is necessary to associate multiple child menus under a parent using a proc for its label.
@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Jul 9, 2012

This pull request passes (merged 06eb8252 into 30b19c8).

This pull request passes (merged 06eb8252 into 30b19c8).

@gregbell

This comment has been minimized.

Show comment Hide comment
@gregbell

gregbell Jul 10, 2012

Contributor

Hi @daxter,

Maybe we could implement it so that the parent key takes a string or a hash. For example:

menu :label => 'Users', :priority => 0, :parent => { :label => "Administration", :priority => "13" }

This way we don't have to duplicate the key extraction and can just pass along the hash to the MenuItem

Contributor

gregbell commented Jul 10, 2012

Hi @daxter,

Maybe we could implement it so that the parent key takes a string or a hash. For example:

menu :label => 'Users', :priority => 0, :parent => { :label => "Administration", :priority => "13" }

This way we don't have to duplicate the key extraction and can just pass along the hash to the MenuItem

@seanlinsley

This comment has been minimized.

Show comment Hide comment
@seanlinsley

seanlinsley Jul 10, 2012

Owner

Okay, here are my thoughts:

Only have one object

@parent_menu_item = options.delete(:parent)

Then define resource.parent_menu_item as such:

def parent_menu_item
  case @parent_menu_item
  when String
    {
      :label => @parent_menu_item,
      :id =>    @parent_menu_item,
      :url =>   '#'
    }
  when Hash
    {
      :label =>    ActiveAdmin::Resource::Name.new(nil, @parent_menu_item[:label]),
      :priority => @parent_menu_item[:priority],
      :url =>      @parent_menu_item[:link] || '#',
      :id =>       @parent_menu_item[:label]
    }
  end
end

I don't know if it's necessary to define the ID as such; I'm just following the precedent of the previous code.

Owner

seanlinsley commented Jul 10, 2012

Okay, here are my thoughts:

Only have one object

@parent_menu_item = options.delete(:parent)

Then define resource.parent_menu_item as such:

def parent_menu_item
  case @parent_menu_item
  when String
    {
      :label => @parent_menu_item,
      :id =>    @parent_menu_item,
      :url =>   '#'
    }
  when Hash
    {
      :label =>    ActiveAdmin::Resource::Name.new(nil, @parent_menu_item[:label]),
      :priority => @parent_menu_item[:priority],
      :url =>      @parent_menu_item[:link] || '#',
      :id =>       @parent_menu_item[:label]
    }
  end
end

I don't know if it's necessary to define the ID as such; I'm just following the precedent of the previous code.

@pcreux

This comment has been minimized.

Show comment Hide comment
@pcreux

pcreux Jul 12, 2012

Contributor

Could you add specs please?

Contributor

pcreux commented Jul 12, 2012

Could you add specs please?

@seanlinsley

This comment has been minimized.

Show comment Hide comment
@seanlinsley

seanlinsley Jul 12, 2012

Owner

@pcreux, working on it now... mind explaining to me the purpose of this spec?

      context "with many parents" do
        before(:each) do
          c1 = MenuItem.new(:label => "C1")
          c2 = MenuItem.new(:label => "C2")
          c3 = MenuItem.new(:label => "C3")

          item.add(c1)
          c1.add c2
          c2.add c3

          item
        end
        let(:sub_item){ item["C1"]["C2"]["C3"] }
        it "should return an array with the parents in reverse order" do
          sub_item.ancestors.should == [item["C1"]["C2"], item["C1"], item]
        end
      end

Since when have we had multilevel nesting in the menu bar? I tried making a child of a child in my app, but it just created a top-level instance of my child menu item. Why are we testing for an unused feature?

Owner

seanlinsley commented Jul 12, 2012

@pcreux, working on it now... mind explaining to me the purpose of this spec?

      context "with many parents" do
        before(:each) do
          c1 = MenuItem.new(:label => "C1")
          c2 = MenuItem.new(:label => "C2")
          c3 = MenuItem.new(:label => "C3")

          item.add(c1)
          c1.add c2
          c2.add c3

          item
        end
        let(:sub_item){ item["C1"]["C2"]["C3"] }
        it "should return an array with the parents in reverse order" do
          sub_item.ancestors.should == [item["C1"]["C2"], item["C1"], item]
        end
      end

Since when have we had multilevel nesting in the menu bar? I tried making a child of a child in my app, but it just created a top-level instance of my child menu item. Why are we testing for an unused feature?

@seanlinsley

This comment has been minimized.

Show comment Hide comment
@seanlinsley

seanlinsley Jul 12, 2012

Owner

@gregbell, is there a reason why we have an ID that's a plural form of the resource name? It seems to make much more sense to just use the label.

Owner

seanlinsley commented Jul 12, 2012

@gregbell, is there a reason why we have an ID that's a plural form of the resource name? It seems to make much more sense to just use the label.

@seanlinsley

This comment has been minimized.

Show comment Hide comment
@seanlinsley

seanlinsley Jul 12, 2012

Owner

@pcreux, I can't figure out how to add specs for my PR as it's not adding a core feature, but allowing it to be accessed by the user.

If I were to define it in spec/unit/menu_item_spec.rb that would be most pointless of all, because that doesn't exercise the frontend code declaration. All that would prove is that priorities work, which is not in any way unique to my code.

Owner

seanlinsley commented Jul 12, 2012

@pcreux, I can't figure out how to add specs for my PR as it's not adding a core feature, but allowing it to be accessed by the user.

If I were to define it in spec/unit/menu_item_spec.rb that would be most pointless of all, because that doesn't exercise the frontend code declaration. All that would prove is that priorities work, which is not in any way unique to my code.

@pcreux

This comment has been minimized.

Show comment Hide comment
@pcreux

pcreux Jul 13, 2012

Contributor

@daxter A feature then? Something that tests the DSL.

Contributor

pcreux commented Jul 13, 2012

@daxter A feature then? Something that tests the DSL.

@seanlinsley

This comment has been minimized.

Show comment Hide comment
@seanlinsley

seanlinsley Jul 27, 2012

Owner

Huh... @travisbot didn't run after I added the tests. I wonder if there's a way to request a re-run.

Owner

seanlinsley commented Jul 27, 2012

Huh... @travisbot didn't run after I added the tests. I wonder if there's a way to request a re-run.

@pcreux

This comment has been minimized.

Show comment Hide comment
@pcreux

pcreux Jul 27, 2012

Contributor

@daxter You could rebase your changes against master and push.

Contributor

pcreux commented Jul 27, 2012

@daxter You could rebase your changes against master and push.

@rainchen

This comment has been minimized.

Show comment Hide comment
@rainchen

rainchen Sep 11, 2012

Is this feature ready to merge into master?

Is this feature ready to merge into master?

@rainchen

This comment has been minimized.

Show comment Hide comment
@rainchen

rainchen Sep 12, 2012

I found a simple way to sort grouped menus:

create app/admin/admin_menus.rb (using this file name to make sure it's the first file get loaded in admin/ dir)

  ActiveAdmin.register_page "CMS" do
    menu :label => "CMS", :priority => 2, :url => '#'
  end

  ActiveAdmin.register_page "CRM" do
    menu :label => "CRM", :priority => 3, :url => '#'
  end
end

Then we can register resource menu using :parent option as normal:

app/admin/pages.rb

ActiveAdmin.register Page do
  menu :label => 'Page', :parent => "CMS"
end

app/admin/user.rb

ActiveAdmin.register User do
  menu :label => 'User', :parent => "CRM"
end

I think this tips should be added to the docs.

I found a simple way to sort grouped menus:

create app/admin/admin_menus.rb (using this file name to make sure it's the first file get loaded in admin/ dir)

  ActiveAdmin.register_page "CMS" do
    menu :label => "CMS", :priority => 2, :url => '#'
  end

  ActiveAdmin.register_page "CRM" do
    menu :label => "CRM", :priority => 3, :url => '#'
  end
end

Then we can register resource menu using :parent option as normal:

app/admin/pages.rb

ActiveAdmin.register Page do
  menu :label => 'Page', :parent => "CMS"
end

app/admin/user.rb

ActiveAdmin.register User do
  menu :label => 'User', :parent => "CRM"
end

I think this tips should be added to the docs.

@xhh

This comment has been minimized.

Show comment Hide comment
@xhh

xhh Oct 18, 2012

👍 how is this going?

xhh commented Oct 18, 2012

👍 how is this going?

@ghost

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Nov 19, 2012

I am very excited about this feature. Any status updates? 😺

ghost commented Nov 19, 2012

I am very excited about this feature. Any status updates? 😺

@seanlinsley

This comment has been minimized.

Show comment Hide comment
@seanlinsley

seanlinsley Dec 2, 2012

Owner

@rainchen, @xhh, @angeltinfoil

Sorry this took so long! The functionality was already finished, but we originally had some problems with Travis CI and I didn't get around to rebasing my commits.

It's been so long, though, that I had to manually rebase this PR so it was up to date with the codebase.

Owner

seanlinsley commented Dec 2, 2012

@rainchen, @xhh, @angeltinfoil

Sorry this took so long! The functionality was already finished, but we originally had some problems with Travis CI and I didn't get around to rebasing my commits.

It's been so long, though, that I had to manually rebase this PR so it was up to date with the codebase.

@seanlinsley

This comment has been minimized.

Show comment Hide comment
@seanlinsley

seanlinsley Dec 2, 2012

Owner

@pcreux, could you take a look at why this test is failing?

The problem is that it checks to ensure that a nested link doesn't somehow also get defined as a top-level (non-nested) link. Child links reside in "#tabs > li > ul > li > a", so they shouldn't be found by the test below.

page.should have_css('#tabs > li > a', :text => name)
Owner

seanlinsley commented Dec 2, 2012

@pcreux, could you take a look at why this test is failing?

The problem is that it checks to ensure that a nested link doesn't somehow also get defined as a top-level (non-nested) link. Child links reside in "#tabs > li > ul > li > a", so they shouldn't be found by the test below.

page.should have_css('#tabs > li > a', :text => name)
@pcreux

This comment has been minimized.

Show comment Hide comment
@pcreux

pcreux Dec 3, 2012

Contributor

@pcreux I don't have time to dig into this test failure those days. Anyone else to help @daxter ?

Contributor

pcreux commented Dec 3, 2012

@pcreux I don't have time to dig into this test failure those days. Anyone else to help @daxter ?

allow parent menu items to be passed settings
supported settings are label, priority, url, and id

+ removed the duplicate merge of default_menu_options
+ clarified MenuItem specs as makes sense semantically
@seanlinsley

This comment has been minimized.

Show comment Hide comment
@seanlinsley

seanlinsley Dec 8, 2012

Owner

I've fixed the problem with my tests. I noticed that the default site created already included an Admin User in the menu bar, so my test checking that "User" wasn't a top-level menu understandably failed. Changed my tests to use "Parent" and "Child".

Owner

seanlinsley commented Dec 8, 2012

I've fixed the problem with my tests. I noticed that the default site created already included an Admin User in the menu bar, so my test checking that "User" wasn't a top-level menu understandably failed. Changed my tests to use "Parent" and "Child".

support procs for parent label
Note that to have more than one child menu item nested under a parent
(when using a proc), you need to pass an id to associate them both.

This was referenced Mar 25, 2013

gregbell added a commit that referenced this pull request Apr 3, 2013

Added parent menu item customization docs
Added docs to help explain what was created in the pull request #1488
@gregbell

This comment has been minimized.

Show comment Hide comment
@gregbell

gregbell Apr 3, 2013

Contributor

I added docs for sorting the parent menu items in 3c02613. The recently merged PRs for the menu system implement this feature.

Contributor

gregbell commented Apr 3, 2013

I added docs for sorting the parent menu items in 3c02613. The recently merged PRs for the menu system implement this feature.

@gregbell gregbell closed this Apr 3, 2013

@seanlinsley

This comment has been minimized.

Show comment Hide comment
@seanlinsley

seanlinsley Apr 3, 2013

Owner

So we're expecting people to define complex menu systems from the AA initializer?

Owner

seanlinsley commented Apr 3, 2013

So we're expecting people to define complex menu systems from the AA initializer?

@gregbell

This comment has been minimized.

Show comment Hide comment
@gregbell

gregbell Apr 3, 2013

Contributor

If you want to store the code elsewhere, you certainly can. However it will be triggered via the initializer.

The problem that you run in to when doing it in app/admin resources is that each resource can clobber the other's parent's definition. So if there are 5 resources that all use the parent "Blog", then which one wins for configuring it?

Configuring parent menu items in the initializer gives the application one place to configure these menu items.

Contributor

gregbell commented Apr 3, 2013

If you want to store the code elsewhere, you certainly can. However it will be triggered via the initializer.

The problem that you run in to when doing it in app/admin resources is that each resource can clobber the other's parent's definition. So if there are 5 resources that all use the parent "Blog", then which one wins for configuring it?

Configuring parent menu items in the initializer gives the application one place to configure these menu items.

@seanlinsley

This comment has been minimized.

Show comment Hide comment
@seanlinsley

seanlinsley Apr 3, 2013

Owner

Yeah, I definitely understand. I realized that problem when putting this together.

One problem that hasn't really been dealt with is: what if the parent has a Proc label? Ultimately you're going to have to associate with the ID. Using the current syntax, there's no way to associate with the parent by its ID. That's one thing that this PR provides.

Owner

seanlinsley commented Apr 3, 2013

Yeah, I definitely understand. I realized that problem when putting this together.

One problem that hasn't really been dealt with is: what if the parent has a Proc label? Ultimately you're going to have to associate with the ID. Using the current syntax, there's no way to associate with the parent by its ID. That's one thing that this PR provides.

@gregbell

This comment has been minimized.

Show comment Hide comment
@gregbell

gregbell Apr 3, 2013

Contributor

Yes, this might be a good idea to add to the Docs. The item id is one of the supported options that you can pass in to a menu item:

https://github.com/gregbell/active_admin/blob/master/lib/active_admin/menu_item.rb#L18

Contributor

gregbell commented Apr 3, 2013

Yes, this might be a good idea to add to the Docs. The item id is one of the supported options that you can pass in to a menu item:

https://github.com/gregbell/active_admin/blob/master/lib/active_admin/menu_item.rb#L18

@seanlinsley

This comment has been minimized.

Show comment Hide comment
@seanlinsley

seanlinsley Apr 3, 2013

Owner

Well of course you can pass it for a menu item, but you can't currently pass an ID to associate a menu item with a parent.

Owner

seanlinsley commented Apr 3, 2013

Well of course you can pass it for a menu item, but you can't currently pass an ID to associate a menu item with a parent.

@gregbell

This comment has been minimized.

Show comment Hide comment
@gregbell

gregbell Apr 3, 2013

Contributor

When you call #menu, it assumes that the :parent option is a menu item id. So this will work:

# 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
Contributor

gregbell commented Apr 3, 2013

When you call #menu, it assumes that the :parent option is a menu item id. So this will work:

# 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
@seanlinsley

This comment has been minimized.

Show comment Hide comment
@seanlinsley

seanlinsley Apr 3, 2013

Owner

head-desk

I don't know how I never realized that. I've analysed and re-written AA menu logic so many times...

I mean I wrote the current revision of MenuNode and MenuItem after all.

Owner

seanlinsley commented Apr 3, 2013

head-desk

I don't know how I never realized that. I've analysed and re-written AA menu logic so many times...

I mean I wrote the current revision of MenuNode and MenuItem after all.

@seanlinsley

This comment has been minimized.

Show comment Hide comment
@seanlinsley

seanlinsley Apr 3, 2013

Owner

But yeah, we should definitely have such an example in our docs.

Owner

seanlinsley commented Apr 3, 2013

But yeah, we should definitely have such an example in our docs.

@seanlinsley seanlinsley deleted the seanlinsley:1283-parent-menuitem-priority branch Apr 18, 2013

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