codeplant/simple-navigation

Blank url and highligh_on_subpath = true causes error.

gondalez opened this issue · 4 comments

Thanks for the neat solution :)
I have hit a small (but blocking) bug!

Repro steps

simple-navigation v4.0.3
mri 2.2.1p85
rails 4.2.3

  • configure multiple groups of items whose parents have no URL (are just groupings)
  • set highlight_on_subpath to true

Observations

  • setting highlight_on_subpath to false stops the error, but using highlights_on is much less fun!
  • setting the url of the parent stops the error, but the parent items are of course links then
  • setting the url of the second parent also stops the error

Example

# config/tasty_nav.rb
SimpleNavigation::Configuration.run do |navigation|
  # this is important
  navigation.highlight_on_subpath = true

  navigation.items do |primary|
    primary.item :first_id, 'first' do |sub_nav| # note no url argument
      sub_nav.item :first_child_id, 'first child', 'some url'
    end

    primary.item :second_id, 'second' do |sub_nav| # no url argument here either
      sub_nav.item :second_child_id, 'first child', 'some other url'
    end
  end
end

And render it like this

<%= render_navigation context: :tasty_nav, expand_all: true %>

(I tried disabling expand_all as well, but no effect)

Error

TypeError: no implicit conversion of nil into String
simple-navigation (4.0.3) lib/simple_navigation/item.rb:177:in `escape'
simple-navigation (4.0.3) lib/simple_navigation/item.rb:177:in `selected_by_subpath?'
simple-navigation (4.0.3) lib/simple_navigation/item.rb:163:in `autohighlight_by_subpath?'
simple-navigation (4.0.3) lib/simple_navigation/item.rb:159:in `selected_by_autohighlight?'
simple-navigation (4.0.3) lib/simple_navigation/item.rb:104:in `selected_by_condition?'
simple-navigation (4.0.3) lib/simple_navigation/item.rb:45:in `selected?'
simple-navigation (4.0.3) lib/simple_navigation/item_container.rb:109:in `any?'
simple-navigation (4.0.3) lib/simple_navigation/item_container.rb:109:in `selected?'
simple-navigation (4.0.3) lib/simple_navigation/item.rb:99:in `selected_by_subnav?'
simple-navigation (4.0.3) lib/simple_navigation/item.rb:45:in `selected?'
simple-navigation (4.0.3) lib/simple_navigation/item.rb:73:in `selected_class'
simple-navigation (4.0.3) lib/simple_navigation/item.rb:55:in `html_options'
simple-navigation (4.0.3) lib/simple_navigation/renderer/list.rb:32:in `block in list_content'
simple-navigation (4.0.3) lib/simple_navigation/renderer/list.rb:31:in `map'
simple-navigation (4.0.3) lib/simple_navigation/renderer/list.rb:31:in `list_content'
simple-navigation (4.0.3) lib/simple_navigation/renderer/list.rb:23:in `render'
simple-navigation (4.0.3) lib/simple_navigation/item_container.rb:103:in `render'
simple-navigation (4.0.3) lib/simple_navigation/helpers.rb:88:in `render_navigation'
... 

https://github.com/codeplant/simple-navigation/blob/master/lib/simple_navigation/item.rb#L177

andi commented

Hi @gondalez

time is currently very limited, if this is blocking you, it would be awesome if you could investigate yourself, maybe add a test that reproduces this behaviour.

We will look into this, but it might take a while..

Cheers
Andi

Hi @andi
No probs; I too am short on time so added this issue while I had the detail, with the intent to come back and code a fix some day.

I'm not blocked any more.. the workaround/hack I went with was to set the top level nav urls to "" and implemented my own renderer that renders top level items without links.

I just wanted to add the info I found since I just ran into this today. This issue also happens when using an empty string url in the config. It looks to be because the code to determine if the link should be selected calls #remove_anchors in "lib/simple_navigation/item.rb". This method calls "split('#').first", which returns nil. That causes the method to return nil, which ends up causing a problem in #selected_by_subpath? when you try to escape the URL.

The #remove_query_params method may also have the same issue, since it also does "split('?').first"

If they did something like:

def remove_anchors(url_with_anchors)
  url_with_anchors && url_with_anchors.split('#').first || ''
end

They would just return an empty string, and I think the escape call would be fine with it.

They would just return an empty string, and I think the escape call would be fine with it.

It would be fine, but it would yield the wrong result, i.e. items with empty url would be all highlighted according to subpath. See linked pr for a simple solution.