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.

Sign up for GitHub

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

Jump to bottom

Fixes #1986 - Menu should register child regardless of AA resource load order #1988

Closed
wants to merge 1 commit into from
Closed

Fixes #1986 - Menu should register child regardless of AA resource load order #1988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions features/menu.feature
View file
Open in desktop
Expand Up @@ -37,3 +37,17 @@ Feature: Menu
Then I should see a menu item for "Custom Menu"
When I follow "Custom Menu"
Then I should be on the admin dashboard page

Scenario: Adding a resource as a sub menu item
Given a configuration of:
"""
ActiveAdmin.register User
ActiveAdmin.register Post do
menu :parent => 'User'
end
"""
When I am on the dashboard
Then I should see a menu item for "Users"
When I follow "Users"
Then the "Users" tab should be selected
And I should see a nested menu item for "Posts"
4 changes: 4 additions & 0 deletions features/step_definitions/menu_steps.rb
View file
Open in desktop
Expand Up @@ -5,3 +5,7 @@
Then /^I should not see a menu item for "([^"]*)"$/ do |name|
page.should_not have_css('#tabs li a', :text => name)
end

Then /^I should see a nested menu item for "([^"]*)"$/ do |name|
page.should have_css('#tabs > li > ul > li > a', :text => name)
end
26 changes: 23 additions & 3 deletions lib/active_admin/menu.rb
View file
Open in desktop
Expand Up @@ -61,7 +61,22 @@ def items

def add_without_parent(item_options)
menu_item = ActiveAdmin::MenuItem.new(item_options, self)

#
# Depending on load order, might have an "empty" parent item already
# in the menu, that is just a label. We want to allow you to customize that if
# you are registering a resource that comes later, such as "Users" parent and
# "Posts" child. The "Users" menu is registered after posts, but posts said it
# already was a sub menu of users.
#
# This will correct that by assigning existing children to this new menu item.
#
if children.has_key? menu_item.id
menu_item.send :children=, self[menu_item.id].send(:children)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

What's with all the sends? Wouldn't this work?

menu_item.children = self[menu_item.id].children

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Private method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Private methods are only private if called out of context. A menu item can call its own "private" methods without any sending.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nevermind, I see this code just created a new menu item object.


children[menu_item.id] = menu_item

end

def add_with_parent(parent, item_options)
Expand All @@ -75,13 +90,18 @@ def children
@children ||= {}
end

def children=(kids)
@children = kids
end
Copy link
Contributor

Choose a reason for hiding this comment

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

We now have this:

def children
  @children ||= {}
end

def children=(kids)
  @children = kids
end

Why not replace both of these with attr_accessor?

attr_accessor :children

Is there a real need to initialize @children with a hash?

Since AA depends on Rails, any places in the code base that checks if there are any children can do this instead:

children.present? # same as !children.blank?

And for inline operations:

children.presence.try :to_do_something

Copy link
Contributor

Choose a reason for hiding this comment

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

Disregard the above comment. I implemented what I was looking for in e11fd6c for #2031. It goes something like this:

module MenuNode
  def initialize
    @children = {}
  end
end

class Menu
  include MenuNode
  def initialize
    super
    # ...
  end
end

I'm thinking we'll get your PR pulled in first, then I'll rebase mine.


def normalize_id(string)
case string
raw_id = case string
when Proc
string
string.call rescue string
else
string.to_s.downcase.gsub(" ", "_")
string.to_s
end
raw_id.downcase.gsub( " ", '_' ).gsub( /[^a-z0-9_]/, '' )
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this is more readable:

def normalize_id(id)
  (id.is_a?(Proc) ? id.call : id).to_s.downcase.gsub ' ', '_'
end

Note that I removed both the unconditional rescue and the regex removing non alphanumeric characters. Why are we doing either of those, and why here?

end

end
Expand Down
9 changes: 0 additions & 9 deletions lib/active_admin/menu_item.rb
View file
Open in desktop
Expand Up @@ -53,15 +53,6 @@ def parent?
!parent.nil?
end

def dom_id
case id
when Proc
id
else
id.to_s.gsub( " ", '_' ).gsub( /[^a-z0-9_]/, '' )
end
end

# Returns an array of the ancestory of this menu item
# The first item is the immediate parent fo the item
def ancestors
Expand Down
6 changes: 3 additions & 3 deletions lib/active_admin/views/tabbed_navigation.rb
View file
Open in desktop
Expand Up @@ -40,11 +40,11 @@ def build_menu
end

def build_menu_item(item)
dom_id = case item.dom_id
dom_id = case item.id
when Proc,Symbol
normalize_id call_method_or_proc_on(self, item.dom_id)
normalize_id call_method_or_proc_on(self, item.id)
else
item.dom_id
item.id
end

li :id => dom_id do |li_element|
Expand Down
14 changes: 11 additions & 3 deletions spec/unit/menu_item_spec.rb
View file
Open in desktop
Expand Up @@ -67,6 +67,10 @@ module ActiveAdmin
i
end

it "should contain 5 submenu items" do
item.items.count.should == 5
end

it "should give access to the menu item as an array" do
item['Blog'].label.should == 'Blog'
end
Expand Down Expand Up @@ -121,7 +125,7 @@ module ActiveAdmin
end # accessing ancestory


describe ".generate_item_id" do
describe "#id" do

it "downcases the id" do
MenuItem.new(:id => "Identifier").id.should == "identifier"
Expand All @@ -131,8 +135,12 @@ module ActiveAdmin
MenuItem.new(:id => "An Id").id.should == "an_id"
end

it "should return a proc if label was a proc" do
MenuItem.new(:label => proc{ "Dynamic" }).id.should be_an_instance_of(Proc)
it "should render a proc if label was a proc" do
MenuItem.new(:label => proc{ "Dynamic Id" }).id.should == "dynamic_id"
end

it "should preserve the ID that was set if the label was a proc" do
MenuItem.new(:label => proc{ "Dynamic Id" }, :id => "static_id").id.should == "static_id"
end

end
Expand Down
10 changes: 10 additions & 0 deletions spec/unit/menu_spec.rb
View file
Open in desktop
Expand Up @@ -50,6 +50,16 @@

menu["Admin"]["Projects"].should be_an_instance_of(ActiveAdmin::MenuItem)
end

it "should assign children regardless of resource file load order" do
menu = Menu.new
menu.add :parent => "Users", :label => "Posts"
menu.add :label => "Users", :url => '/some/url'

menu["Users"].url.should == '/some/url'
menu["Users"]["Posts"].should be_an_instance_of(ActiveAdmin::MenuItem)
end

end

end
Expand Down