[2023] Fix MenuItem label sorting #2031

Merged
merged 1 commit into from Apr 2, 2013

Conversation

Projects
None yet
3 participants
Owner

seanlinsley commented Mar 29, 2013

An alternative approach to #2023.

@macfanatic's MenuItem changes in 452956e updated label procs to be evaluated in the view context, but this broke sorting of proc labels. This PR initially moved the sorting logic up into TabbedNavigation, where the view context is accessible. Later, I decided that this was a more elegant solution:

# in the view context, e.g. `TabbedNavigation`
link_to item.label(self), item.url(self), item.html_options

# with this implementation
class MenuItem
  def label(context = nil)
    render_in_context context, @label
  end
end

This is the commit history of this PR from before I removed the outdated commits.

First attempt

  • b67d583 - moved sorting logic into TabbedNavigation to follow the trend
  • bc20972 - renamed a MethodOrProc helper method (no longer included)

Second attempt (adding to the above)

  • 293b4d0 - tried a more OO approach, where you can pass in the view context to MenuItem to use for Procs
  • e11fd6c - moved remaining MenuItem-specfic out of TabbedNavigation ❤️
  • f60ebf7 - integrated fixes from #1988
lib/active_admin/menu.rb
- string.to_s.downcase.gsub(" ", "_")
- end
+ def normalize_id(id)
+ id.is_a?(Proc) ? id : id.to_s.downcase.gsub(' ', '_')
@seanlinsley

seanlinsley Mar 29, 2013

Owner

@macfanatic why are we supporting Procs as IDs? When would that be used?

@macfanatic

macfanatic Mar 29, 2013

Contributor

Was the best way I could think of to support a proc for the label and turn it into a meaningful ID for the DOM.

@seanlinsley

seanlinsley Mar 29, 2013

Owner

When I created #1488 I ran into the same problem. Back then I was thinking that you'd have to set a manual ID alongside the proc, but now I'm thinking... why not use the resource name as the default if none is provided?

Either that or we could raise an exception, forcing developers to add in an ID.

Contributor

exviva commented Mar 29, 2013

@daxter makes sense, thanks.

Owner

seanlinsley commented Mar 30, 2013

@macfanatic I know the tests aren't passing, but could you take a look at the most recent commit (293b4d0)?

I think allowing MenuItem#label and #url to be passed a context gives us the best of both worlds.

Also... maybe I'm missing something... but what exactly is the benefit of evaluating procs & symbols in the view context?

Contributor

macfanatic commented Mar 30, 2013

I'll look at the code later, but the motivation was to allow the user to use view helpers & route methods basically.

Owner

seanlinsley commented Apr 1, 2013

💚

Owner

seanlinsley commented Apr 1, 2013

@macfanatic can you evaluate this PR? Save for updating documentation, it should be finished.

Note that once you give the okay, I'll want to squash these commits into a single one.

- menu["Categories"].display_if_block.should be_instance_of(Proc)
- menu["Categories"].display_if_block.call.should == false
- end
- end # describe "setting a condition for displaying"
@seanlinsley

seanlinsley Apr 1, 2013

Owner

Note that I removed this chunk of code from register_resource_spec and register_page_spec because this stuff is already tested by menu_spec and menu_item_spec.

Contributor

macfanatic commented Apr 1, 2013

I think if you update the docs you mention, that this is good. Seems clean to me.

Owner

seanlinsley commented Apr 1, 2013

@macfanatic I'm trying to update the docs in the MenuItem file with appropriate examples for symbol/proc usage.

For URLs that's simple, because you have access to named routes:

menu url: :root_path

What about symbols/procs for the label? I can't find a good example, especially since the usual helpers are borked.

menu label: ->{ resource_class.to_s } # ActiveAdmin::Page

I suppose access to helper methods at all is a great help, since the dev could build their custom helper to be used, but I'd still like to have a better example of its usage.

Contributor

macfanatic commented Apr 1, 2013

@daxter - i18n.t() call would probably be a great example, that's a view helper, right?

Owner

seanlinsley commented Apr 1, 2013

But the i18n call isn't scoped at all.

menu label: ->{ I18n.t :fake } # translation missing: en.fake

Also it's not a view helper; i18n is available everywhere.

Contributor

macfanatic commented Apr 1, 2013

Good point. How about this then:

menu label: -> { "#{Date.today.strftime('%A')}'s Posts" } # "Monday's Posts"
Owner

seanlinsley commented Apr 1, 2013

Well I suppose. But I was looking for an example that utilized the view context.

Again, the whole proc thing would be more useful if you could access the model. Still don't understand why that doesn't work.

Contributor

macfanatic commented Apr 1, 2013

You should be able to access it via the inherited resources helpers, but only on appropriate actions of course.

menu label: -> { display_name(resource) rescue resource_class.to_s } # uses an AA helper, IH helper & fallback?
Owner

seanlinsley commented Apr 1, 2013

No that doesn't work. The example I gave before:

menu label: ->{ resource_class.to_s } # ActiveAdmin::Page

Hence this commit (c2b235e55) followed by this commit (108925f95).

The code I replaced did an unconditional rescue when evaluating procs, and that's a terrible thing to do. Isn't there any way we can provide both the resource class and the view context without such a hack?

Contributor

macfanatic commented Apr 1, 2013

resource_class might be a method that you can't get in the view layer, but you should be able to get that info through the view layer somehow, starting with active_admin_config?

Owner

seanlinsley commented Apr 2, 2013

The problem is, every proc is evaluated in the context of the page being viewed. For example:

menu label: ->{ controller.class.to_s }
# When on the posts page, this shows PostsController
# When on the users page, this shows UsersController
# etc.
Owner

seanlinsley commented Apr 2, 2013

The only way someone's going to be able to access their model is by calling out the class like so:

menu label: ->{ User.some_method }
Owner

seanlinsley commented Apr 2, 2013

Okay this is what I settled on:

    # NOTE: for :label, :url, and :if
    # These options are evaluated in the view context at render time. Symbols are called
    # as methods on `self`, and Procs are exec'd within `self`.
    # Here are some examples of what you can do:
    #
    #   menu if:  :admin?
    #   menu url: :new_book_path
    #   menu url: :awesome_helper_you_defined
    #   menu label: ->{ User.some_method }
    #   menu label: ->{ I18n.t 'menus.user' }
    #
fix MenuItem label sorting of procs
@macfanatic's changes in 452956e changed label procs to be evaluated in the view context,
but this broke `MenuItem` sorting. This commit pushes the logic back down into `MenuItem`,
and provides a way to pass in an optional context in which to evaluate symbols and procs.

+ forces `MenuItem`-specific logic out of `TabbedNavigation`
+ `items` now handles both sorting and display decisions. Call `children` if you want them all
+ `add` was refactored for clarity
+ integrates fixes from #1988

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

@seanlinsley seanlinsley merged commit c7d05c0 into activeadmin:master Apr 2, 2013

1 check passed

default The Travis build passed
Details

@seanlinsley seanlinsley deleted the seanlinsley:bugfix/2023-menu-sorting branch Apr 3, 2013

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