codeplant / simple-navigation

A ruby gem for creating navigations (with multiple levels) for your Rails, Sinatra or Padrino applications. Render your navigation as html list, link list or breadcrumbs.
http://simple-navigation-demo.codeplant.ch/
MIT License
885 stars 136 forks source link

Rendering dynamic menu with hash changes the hash after first run #153

Closed averissimo closed 10 years ago

averissimo commented 10 years ago

When rendering dynamic menus with a hash, the run will change values in the hash. Making it impossible to reuse the hash.

How to replicate (in a view):

> context_menu = [
        {
            :key => :new, 
            :name => 'random name', 
            :url => url_for([:new,@model,:experiment]),
            :options => {
                :if => Proc.new { can?(:update, @model) },
                :container_class => 'menu',
                :class => "btn"
        },
        {
            :key => :another, 
            :name => 'random name 2', 
            :show_in_top => false,
            :url => url_for([:new,@model,:experiment]),
            :options => {
                :if => Proc.new { can?(:update, @model) },
                :container_class => 'menu',
                :class => "btn"
        }
}]
> menu1 = render_navigation(:items => context_menu.delete_if { |i| i[:show_in_top] == false })
> menu2 = render_navigation( items: context_menu)

menu1 and menu2 don't have the html ul's class (in this case "menu")

Is this behavior expected?

andi commented 10 years ago

So what happens when you only render menu1?

On Thu, Feb 27, 2014 at 8:21 PM, André Veríssimo notifications@github.comwrote:

When rendering dynamic menus with a hash, the run will change values in the hash. Making it impossible to reuse the hash.

How to replicate (in a view):

context_menu = [ { :key => :new, :name => 'random name', :url => url_for([:new,@model,:experiment]), :options => { :if => Proc.new { can?(:update, @model) }, :container_class => 'menu', :class => "btn" }, { :key => :another, :name => 'random name 2', :show_in_top => false, :url => url_for([:new,@model,:experiment]), :options => { :if => Proc.new { can?(:update, @model) }, :container_class => 'menu', :class => "btn" }}]> menu1 = render_navigation(:items => context_menu.delete_if { |i| i[:show_in_top] == false })> menu2 = render_navigation( items: context_menu)

menu1 and menu2 don't have the html ul's class (in this case "menu")

Is this behavior expected?

— Reply to this email directly or view it on GitHubhttps://github.com/codeplant/simple-navigation/issues/153 .

averissimo commented 10 years ago

menu1 works fine and as expected <ul class="menu">...</ul>

menu2 renders <ul>..</ul>.. if I inspect the _contextmenu variable after _rendernavigation is called, I detect that all the options of the different items are empty.

There's a easy solution, by defining the hash as a method _def contextmenu() ... end and call it every time I need to render the menu.

There's probably a reason for this behavior, looks strange for me and I am just wondering.. but it's a minor "thing", as calling more than once should be a pretty rare necessity :)

andi commented 10 years ago

I guess that's not intended behaviour. I will investigate asap… If you find out before, let me know :-)

On Thu, Feb 27, 2014 at 11:13 PM, André Veríssimo notifications@github.comwrote:

menu1 works fine and as expected

menu2 renders

    ..
.. if I inspect the _contextmenu variable after _rendernavigation is called, I detect that all the options of the different items are empty.

There's a easy solution, by defining the hash as a method _def contextmenu() ... end and call it every time I need to render the menu.

There's probably a reason for this behavior, looks strange for me and I am just wondering.. but it's a minor "thing", as calling more than once should be a pretty rare necessity :)

— Reply to this email directly or view it on GitHubhttps://github.com/codeplant/simple-navigation/issues/153#issuecomment-36298266 .

simonc commented 10 years ago

@averissimo I tried you example but with a small modification and it seems to be working fine:

<% context_menu = [{
  key: :new,
  name: 'Create a new user',
  url: url_for([:new, :user]),
  options: {
    container_class: 'menu',
    class: 'btn'
  }
}, {
  key: :another,
  name: 'Create a new cartoon',
  show_in_top: false,
  url: url_for([:new, :cartoon]),
  options: {
    container_class: 'menu',
    class: 'btn'
  }
}]
%>
<%= render_navigation(items: context_menu.select { |i| i[:show_in_top] != false }) %>
<%= render_navigation( items: context_menu) %>
simonc commented 10 years ago

Sorry, I sent my previous message without the code (updated the Github message).

@averissimo are you using the delete_if when rendering? Are you aware that delete_if actually deletes elements on the Array on which it's called? :)

averissimo commented 10 years ago

I was off with that "delete_if" :) .. but the underlying behavior I'm reporting persists.

I.E. some options are cleared from the hash with every render_navigation run (especially: class, if)

With your code (again with minor variation with models) the initial contextmenu has this value (when inspected)_:

[{:key=>:new, :name=>"Create a new user", :url=>"/projects/new", :options=>{:container_class=>"menu", :class=>"btn"}}, {:key=>:another, :name=>"Create a new cartoon", :show_in_top=>false, :url=>"/models/new", :options=>{:container_class=>"menu", :class=>"btn"}}] 

After first run (with reject)

[{:key=>:new, :name=>"Create a new user", :url=>"/projects/new", :options=>{:class=>"btn", :id=>"new"}}, {:key=>:another, :name=>"Create a new cartoon", :show_in_top=>false, :url=>"/models/new", :options=>{:container_class=>"menu", :class=>"btn"}}]

After second run (without reject)

[{:key=>:new, :name=>"Create a new user", :url=>"/projects/new", :options=>{:class=>"btn", :id=>"new"}}, {:key=>:another, :name=>"Create a new cartoon", :show_in_top=>false, :url=>"/models/new", :options=>{:class=>"btn", :id=>"another"}}]

container_class option disapears, but the same happens with the "if" (haven't tracked any other options)

simonc commented 10 years ago

Ok, I see the issue. I'd like to take a shot at another implementation of the whole option handling so I'll keep this in mind while doing so ;)

simonc commented 10 years ago

This issue has been fixed with commit d7f75c037325aa4be78ce747f361bcc0955ffda3

Reopen if this is still problematic.