Refactor [1967] Menu Updates #1987

Merged
merged 1 commit into from Apr 26, 2013

Conversation

Projects
None yet
2 participants
Owner

seanlinsley commented Mar 13, 2013

Sorry @macfanatic, I didn't pay close enough attention to #1967 at the time. This is my attempt to make sense of the changes as I rebase my PR (#1488) onto master.

@macfanatic macfanatic and 1 other commented on an outdated diff Mar 13, 2013

lib/active_admin/resource/menu.rb
@@ -49,27 +47,23 @@ def navigation_menu
namespace.fetch_menu(navigation_menu_name)
end
- def menu_item_menu_name=(menu_name)
- @menu_item_menu_name = menu_name
- end
+ attr_writer :menu_item_name
@macfanatic

macfanatic Mar 13, 2013

Contributor

While using attr_writer is fine, I think renaming menu_item_menu_name to menu_item_name is more confusing, b/c the variable represents the name of the navigation_menu itself, not anything to do with the MenuItem.

@seanlinsley

seanlinsley Mar 13, 2013

Owner

Then why not call it navigation_menu_name or something similar?

@macfanatic

macfanatic Mar 13, 2013

Contributor

Perfect.

@seanlinsley

seanlinsley Mar 13, 2013

Owner

Uh... there's already a navigation_menu_name here. And they both seem to carry the same value...

@seanlinsley seanlinsley and 1 other commented on an outdated diff Mar 13, 2013

lib/active_admin/resource/menu.rb
@menu_item_options = {}
else
- self.menu_item_menu_name = options[:menu_name]
+ self.navigation_menu_name = options[:menu_name]
@seanlinsley

seanlinsley Mar 13, 2013

Owner

Where does options[:menu_name] come from?

@seanlinsley

seanlinsley Mar 19, 2013

Owner

I don't see this ever getting populated, and it's not listed anywhere in the docs...

@macfanatic

macfanatic Mar 19, 2013

Contributor

I'm not 100% sure if it would be called after not looking at this for awhile. I know that the variable itself is used as a key into the hash that is essentially the MenuCollection class. Default key being ActiveAdmin::Menu::DEFAULT_MENU of :default.

@seanlinsley

seanlinsley Mar 19, 2013

Owner

Grepping through the source code, I don't see this ever being passed in.

@seanlinsley

seanlinsley Apr 9, 2013

Owner

Care to take another look at this @macfanatic?

@seanlinsley seanlinsley commented on an outdated diff Mar 13, 2013

lib/active_admin/resource/routes.rb
end
+ # ???
def route_uncountable?
controller.resources_configuration[:self][:route_collection_name] ==
@seanlinsley

seanlinsley Mar 13, 2013

Owner

What does this method do?

Owner

seanlinsley commented Mar 19, 2013

@macfanatic I see how my changes broke things, but I still don't understand the purpose of menu_item_menu_name.

I've been banging my head against the wall for far too long on this. Could you please elaborate on the intent?

Owner

seanlinsley commented Mar 19, 2013

So to start with an example, with this configuration, what's instantiated?

ActiveAdmin.register User
ActiveAdmin.register Post do
  belongs_to :user
end

What does the menu item hold for Post?

What's different between the above, and the below (in terms of values stored)?

belongs_to :user
navigation_menu :user
Contributor

macfanatic commented Mar 19, 2013

  1. belongs_to is a DSL to setup routes, breadcrumbs, etc.
  2. navigation_menu is a symbol or proc that sets up an entire menu system.

With the the first scenario, Post & User are still both in the same navigation menu. However, you could setup an entirely different menu to be used when accessing User records, for instance. Idea being that the menu would be more "contextual", the prime example being a Project that had tickets, milestones, etc...so you would have a nice detailed navigation menu when inside the project itself, but the global menu would just be registered with the :default navigation menu.

Does that help at all?

Owner

seanlinsley commented Mar 19, 2013

I'm not really stuck on the navigation_menu; it's the menu_item_menu_name that's confusing things.

Owner

seanlinsley commented Mar 19, 2013

Specifically, menu_item_menu_name is set to :users in the belongs_to :user example.

And that's used to... effectively not render a menu item at all... which doesn't make any sense. Why not remove the menu item entirely from Post and just reference the User menu item?

Owner

seanlinsley commented Mar 19, 2013

Ultimately, I don't understand the purpose of menu_item_menu_name

Owner

seanlinsley commented Mar 19, 2013

@macfanatic still alive?

Contributor

macfanatic commented Apr 10, 2013

@daxter - Let's get on skype and work through this. Sent you a DM on twitter with my handle to get this rolling.

seanlinsley was assigned Apr 10, 2013

Owner

seanlinsley commented Apr 25, 2013

@macfanatic the only tests that are failing with this appear to be out of date... you tell me.

Given this configuration:

ActiveAdmin.register User
ActiveAdmin.register(Post) { belongs_to :user }

And the changes in this PR, you get a unique navigation menu for the belongs_to association:
belongs_to_menu

As opposed to the full list of menu items showing up, with post not showing up at all

Owner

seanlinsley commented Apr 26, 2013

@macfanatic since you're around right now, thoughts on this?

The commits obviously need better names, but this should be ready to merge if you agree with the direction I took.

Contributor

macfanatic commented Apr 26, 2013

Looks good to me.

@seanlinsley @seanlinsley seanlinsley refactor menu item attributes
This commit merges `menu_item_menu_name` into `navigation_menu_name`, as
they both appear to have the same purpose. By doing so, this seems to fix a
previously unreported issue of belongs_to nested resources. Now, the menu
items for those nested resources show up in a unique navigation menu
specific to the parent resource.
5101f78
Owner

seanlinsley commented Apr 26, 2013

Okay, updated.

@seanlinsley seanlinsley added a commit that referenced this pull request Apr 26, 2013

@seanlinsley seanlinsley Merge pull request #1987 from Daxter/refactor/1967-menu-updates
Refactor [1967] Menu Updates
188593a

@seanlinsley seanlinsley merged commit 188593a into activeadmin:master Apr 26, 2013

1 check passed

default The Travis build passed
Details

seanlinsley deleted the unknown repository branch Apr 26, 2013

Owner

seanlinsley commented May 12, 2013

Reading through the docs for #2172, I noticed this section that I seem to have broken...

cc @gregbell, @macfanatic

Belongs To

It's common to want to scope a series of resources to a relationship. For
example a Project may have many Milestones and Tickets. To nest the resource
within another, you can use the belongs_to method:

ActiveAdmin.register Project

ActiveAdmin.register Ticket do
  belongs_to :project
end

Projects will be available as usual and tickets will be availble by visiting
/admin/projects/1/tickets assuming that a Project with the id of 1 exists.
Active Admin adds the child resources to a child navigation menu that only shows
up when viewing a child.

To create links to the resource, you can add them to a sidebar (one of the many
possibilities for how you may with to handle your user interface):

ActiveAdmin.register Project do
  sidebar "Project Details" do
    ul do
      li link_to("Tickets", admin_project_tickets_path(project))
      li link_to("Milestones", admin_project_milestones_path(project))
    end
  end
end

ActiveAdmin.register Ticket do
  belongs_to :project
end

ActiveAdmin.register Milestone do
  belongs_to :project
end

In some cases (like Projects), there are many sub resources and you would
actually like the global navigation to switch when the user navigates "into" a
project. To accomplish this, Active Admin stores the belongs_to resources in a
seperate menu which you can use if you so wish. To use:

ActiveAdmin.register Ticket do
  belongs_to :project
  navigation_menu :project
end

ActiveAdmin.register Milestone do
  belongs_to :project
  navigation_menu :project
end

Now, when you navigate to the tickets section, the global navigation will
only display "Tickets" and "Milestones". When you navigate back to a
non-belongs_to resource, it will switch back to the default menu.

Contributor

macfanatic commented May 12, 2013

What's broke about it? I'm updating SprintApp to use AA master and sub-resource menu changes appear to work fine. I switched them back to ":default" and used "menu false" since I don't want them shown, but worked for me.

On May 12, 2013, at 5:34 PM, Sean Linsley notifications@github.com wrote:

Reading through the docs for #2172, I noticed this section that I seem to have broken...

cc @gregbell, @macfanatic

Belongs To

It's common to want to scope a series of resources to a relationship. For
example a Project may have many Milestones and Tickets. To nest the resource
within another, you can use the belongs_to method:

ActiveAdmin.register Project

ActiveAdmin.register Ticket do
belongs_to :project
end
Projects will be available as usual and tickets will be availble by visiting
/admin/projects/1/tickets assuming that a Project with the id of 1 exists.
Active Admin adds the child resources to a child navigation menu that only shows
up when viewing a child.

To create links to the resource, you can add them to a sidebar (one of the many
possibilities for how you may with to handle your user interface):

ActiveAdmin.register Project do
sidebar "Project Details" do
ul do
li link_to("Tickets", admin_project_tickets_path(project))
li link_to("Milestones", admin_project_milestones_path(project))
end
end
end

ActiveAdmin.register Ticket do
belongs_to :project
end

ActiveAdmin.register Milestone do
belongs_to :project
end
In some cases (like Projects), there are many sub resources and you would
actually like the global navigation to switch when the user navigates "into" a
project. To accomplish this, Active Admin stores the belongs_to resources in a
seperate menu which you can use if you so wish. To use:

ActiveAdmin.register Ticket do
belongs_to :project
navigation_menu :project
end

ActiveAdmin.register Milestone do
belongs_to :project
navigation_menu :project
end
Now, when you navigate to the tickets section, the global navigation will
only display "Tickets" and "Milestones". When you navigate back to a
non-belongs_to resource, it will switch back to the default menu.


Reply to this email directly or view it on GitHub.

Owner

seanlinsley commented May 12, 2013

The docs say that a simple belongs_to :foo shouldn't create a parent navigation menu. My changes in this PR make that the default.

Owner

seanlinsley commented Sep 6, 2013

Coming back to this PR... can we have this discussion again @macfanatic?

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