Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

fixing disappearing options #91

Closed
wants to merge 6 commits into from

4 participants

@suung

hey andi,

problem was:

1) instance variable with config
2) several render calls with different levels
3) first call changes the config removing options.

this seems to fix it.

chris trying to fix the problem that simple navigation being called twice r…
…emoves data out of the config stored in an instance variable
2b2ffad
@mjtko
Collaborator

Hi @suung,

I'll likely merge this, though it would be nice if it had been accompanied by some updated specs too! :-)

Cheers,

Mark.

@mjtko
Collaborator

Hi @suung,

I'm trying to determine under exactly what conditions the problem you mention is occurring.

You mention an 'instance variable with config' - what variable are you referring to here?

Also, you mention 'several render calls' - I guess you mean that you're calling render_navigation multiple times in different places in a template.

I would like to get this fixed up so it doesn't cause the problems you're encountering, but without tests or further explanation I'm nervous that doing what is possibly half the job might be worse!

If you could get back to me with a few more details, that'd be much appreciated.

Cheers,

Mark.

@suung

hey mark,

sorry for the delay.

yeah, i am not sure how to test this, but will work somehow.

i currently ran in the same problem, where someone forgot to use our patch.
that's the setup:

- debugger
= render "edit_navigation"
- debugger

where

 debugger
(rdb:13) p @main_navigation
[{:key=>"main_navigation_assignment", :name=>"My Assignments", :url=>"/assignments", :options=>{:section=>"left", :if=>#<Proc:0x007ffbe86aaec8@/home/suung/workspace/development/tarantula/vendor/plugins/devolute-rails-navigation/lib/devolute/patterns/navigation.rb:209 (lambda)>}}, {:key=>"main_navigation_prospect", :name=>"Dispatching", :url=>"/prospects", :options=>{:section=>"left", :if=>#<Proc:0x007ffbe86f51d0@/home/suung/workspace/development/tarantula/vendor/plugins/devolute-rails-navigation/lib/devolute/patterns/navigation.rb:209 (lambda)>}}, {:key=>"main_navigation_review", :name=>"Final Cut", :url=>"/reviews", :options=>{:section=>"left", :if=>#<Proc:0x007ffbe8717d48@/home/suung/workspace/development/tarantula/vendor/plugins/devolute-rails-navigation/lib/devolute/patterns/navigation.rb:209 (lambda)>}}, {:key=>"main_navigation_user_session", :name=>"Sign Out", :url=>"/users/sign_out", :options=>{:method=>:delete, :section=>"right", :if=>#<Proc:0x007ffbe874d3d0@/home/suung/workspace/development/tarantula/vendor/plugins/devolute-rails-navigation/lib/devolute/patterns/navigation.rb:209 (lambda)>}}, {:key=>"main_navigation_user_session", :name=>"Sign In", :url=>"/users/sign_in", :options=>{:section=>"right", :unless=>#<Proc:0x007ffbe876ff20@/home/suung/workspace/development/tarantula/vendor/plugins/devolute-rails-navigation/lib/devolute/patterns/navigation.rb:209 (lambda)>}}, {:key=>"main_navigation_user", :name=>"⚒", :url=>"/users", :options=>{:section=>"right", :if=>#<Proc:0x007ffbe8791e68@/home/suung/workspace/development/tarantula/vendor/plugins/devolute-rails-navigation/lib/devolute/patterns/navigation.rb:209 (lambda)>}, :items=>[{:key=>"user_navigation_level_2_show", :name=>"Show", :url=>"/users/1", :options=>{}}, {:key=>"user_navigation_level_2_new", :name=>"New", :url=>"/users/new", :options=>{}}, {:key=>"user_navigation_level_2_edit", :name=>"Edit", :url=>"/users/1/edit", :options=>{}}, {:key=>"navigation_level_2_back", :name=>"Back", :url=>"http://localhost:3000/"}]}, {:name=>"☺", :key=>"main_navigation_edit_profile", :url=>"/users/1/edit", :options=>{:section=>"right"}}]
(rdb:13) n
/home/suung/workspace/development/tarantula/app/views/users/_edit.html.haml:3
- if resource.errors.any?
(rdb:13) p @main_navigation
[{:key=>"main_navigation_assignment", :name=>"My Assignments", :url=>"/assignments", :options=>{:section=>"left"}}, {:key=>"main_navigation_prospect", :name=>"Dispatching", :url=>"/prospects", :options=>{:section=>"left"}}, {:key=>"main_navigation_review", :name=>"Final Cut", :url=>"/reviews", :options=>{:section=>"left"}}, {:key=>"main_navigation_user_session", :name=>"Sign Out", :url=>"/users/sign_out", :options=>{:section=>"right"}}, {:key=>"main_navigation_user_session", :name=>"Sign In", :url=>"/users/sign_in", :options=>{:section=>"right"}}, {:key=>"main_navigation_user", :name=>"⚒", :url=>"/users", :options=>{:section=>"right"}, :items=>[{:key=>"user_navigation_level_2_show", :name=>"Show", :url=>"/users/1", :options=>{}}, {:key=>"user_navigation_level_2_new", :name=>"New", :url=>"/users/new", :options=>{}}, {:key=>"user_navigation_level_2_edit", :name=>"Edit", :url=>"/users/1/edit", :options=>{}}, {:key=>"navigation_level_2_back", :name=>"Back", :url=>"http://localhost:3000/"}]}, {:name=>"☺", :key=>"main_navigation_edit_profile", :url=>"/users/1/edit", :options=>{:section=>"right"}}]
(rdb:13) 

what this means is:

normally you would render one navigation once. and another somewhere else.
we do it twice. so we notice, the variable changes

if i can breath again and my code gets stable, then i will certainly come back to you with a test

@mjtko
Collaborator

Hi @suung,

Any sign of that test yet? :-)

Cheers,

Mark.

@suung
@mjtko mjtko was assigned
@andi
Collaborator

I think I don't understand the problem here. Can you briefly wrap up your setup again and explain exactly what the problem is?

Thanks
Andi

@suung

hey andi, good you are back.

basically in the first render call, stuff gets .delete(:bla) out of the options hash. if, for some reason, you might render the same (dynamic) navigation twice, you loose it. makes sense, if you think about it.

cheers

@simonc
Collaborator

If I understand the problem. Calling render_navigation twice was causing the conditions not to be applied on the second call.

If that's precisely the issue here, it's working correctly with the current implementation.

@suung

most likely it has been fixed. I don't know, there was a

options.delete(...) scenario in a nesting above 1 level.

it is very obvious in the code. if it is fixed, it is not in the code anymore, maybe someone can take care for this.

anyway here is my work including tests. https://github.com/devolute/simple-navigation/commits/master
we switched to an own implementation,

@simonc
Collaborator

I created an application with the following navigation:

SimpleNavigation::Configuration.run do |navigation|
  navigation.items do |primary|
    primary.item :users, 'Users', users_path do |users|
      users.item :new, 'Create user', new_user_path, if: proc{ false }
    end
  end
end

And called render_navigation twice in the application layout.
The item is hidden in both menus when the proc is false and shown in both when it is true so I suppose it's working properly.

I'll close this issue, since it seems to be fixed :)

@simonc simonc closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 12, 2012
  1. trying to fix the problem that simple navigation being called twice r…

    chris authored
    …emoves data out of the config stored in an instance variable
Commits on Apr 11, 2012
  1. fixed problem occuring from the previous commit that caused options s…

    chris authored
    …traightly being forwarded to the dom
  2. better fix for the same issue :)

    chris authored
  3. removing section from rendered html_options

    chris authored
Commits on May 15, 2012
  1. passing blocks to the renderer

    chris authored
Commits on May 31, 2012
  1. added test to prove that :ifs and :unless are not removed from variab…

    chris authored
    …les containing the navgiation
This page is out of date. Refresh to see the latest.
View
4 lib/simple_navigation/core/item.rb
@@ -125,8 +125,8 @@ def url_without_anchor
private
def setup_url_and_options(url_or_options, options_or_nil)
- options = options_or_nil
- url = url_or_options
+ options = options_or_nil.dup
+ url = url_or_options.dup
case url_or_options
when Hash
# url_or_options is options (there is no url)
View
4 lib/simple_navigation/core/item_container.rb
@@ -35,12 +35,16 @@ def initialize(level=1) #:nodoc:
#
# The <tt>block</tt> - if specified - will hold the item's sub_navigation.
def item(key, name, url_or_options = {}, options_or_nil = {}, &block)
+ url_or_options = url_or_options.dup if url_or_options
+ options_or_nil = options_or_nil.dup if options_or_nil
options = url_or_options.is_a?(Hash) ? url_or_options : options_or_nil
(@items << SimpleNavigation::Item.new(self, key, name, url_or_options, options_or_nil, nil, &block)) if should_add_item?(options)
end
def items=(items)
items.each do |item|
+ item = item.dup
+ item[:options] = item[:options].dup if item[:options]
item = SimpleNavigation::ItemAdapter.new(item)
(@items << item.to_simple_navigation_item(self)) if should_add_item?(item.options)
end
View
4 lib/simple_navigation/rendering/helpers.rb
@@ -31,8 +31,8 @@ module Helpers
# will be loaded and used for rendering the navigation.
# * <tt>:items</tt> - you can specify the items directly (e.g. if items are dynamically generated from database). See SimpleNavigation::ItemsProvider for documentation on what to provide as items.
# * <tt>:renderer</tt> - specify the renderer to be used for rendering the navigation. Either provide the Class or a symbol matching a registered renderer. Defaults to :list (html list renderer).
- def render_navigation(options={})
- active_navigation_item_container(options) { |container| container && container.render(options) }
+ def render_navigation(options={},&block)
+ active_navigation_item_container(options) { |container| container && container.render(options,&block) }
end
# Returns the name of the currently active navigation item belonging to the specified level.
View
2  lib/simple_navigation/rendering/renderer/list.rb
@@ -11,7 +11,7 @@ module Renderer
class List < SimpleNavigation::Renderer::Base
def render(item_container)
list_content = item_container.items.inject([]) do |list, item|
- li_options = item.html_options.reject {|k, v| k == :link}
+ li_options = item.html_options.reject {|k, v| [:link,:section].include?(k) }
li_content = tag_for(item)
if include_sub_navigation?(item)
li_content << render_sub_navigation_for(item)
View
55 spec/lib/simple_navigation/rendering/helpers_spec.rb
@@ -248,18 +248,51 @@ def whitebox_setup
end
end
- end
-
- describe "should treat :level and :levels options the same" do
- before(:each) do
- whitebox_setup
- @selected_item_container = stub(:selected_container).as_null_object
- SimpleNavigation.stub!(:active_item_container_for => @selected_item_container)
+ describe "should treat :level and :levels options the same" do
+ before(:each) do# whitebox_setup
+ @selected_item_container = stub(:selected_container).as_null_object
+ SimpleNavigation.stub!(:active_item_container_for => @selected_item_container)
+ end
+ it "should pass a valid levels options as level" do
+ @selected_item_container.should_receive(:render).with(:level => 2)
+ @controller.render_navigation(:levels => 2)
+ end
end
- it "should pass a valid levels options as level" do
- @selected_item_container.should_receive(:render).with(:level => 2)
- @controller.render_navigation(:levels => 2)
+
+
+
+ context "with dynamic options stored in instance variable" do
+ before :each do
+
+ class ControllerMock
+
+ attr_accessor :navigation_items
+ attr_accessor :test_lambda
+ def initialize
+ super
+ @test_lambda = lambda { true }
+ @navigation_items = [{ :key => "foo", :url => "/bar", :name => "foo", :options => { :if => @test_lambda }, :items => [:key => "bar", :url => "/boo", :name => "bar", :section => "bla", :options => { :test => true }]}]
+ end
+
+ end
+
+ end
+
+ context "rendering twice" do
+
+ subject { ControllerMock.new}
+
+ it "should not change the configuration after the first rendering (specifically not removing the :if or :unless keys)" do
+ @navigation_items = [{ :key => "foo", :url => "/bar", :name => "foo", :options => { :if => subject.test_lambda }, :items => [:key => "bar", :url => "/boo", :name => "bar", :section => "bla", :options => { :test => true }]}]
+ subject.render_navigation(:items => subject.navigation_items)
+ subject.navigation_items.should == @navigation_items
+ end
+
+ end
+
end
+
end
-
+
+
end
Something went wrong with that request. Please try again.