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 trouble #166

Closed zdravko closed 10 years ago

zdravko commented 10 years ago

render_navigation produces a strange looking link: Label#SimpleNavigation::Item:0x7f59c26fb6c0

Label is ok, but the rest is odd. Could it be that I am trying to run simple-navigation on ruby 1.8? I changed a lot of hash syntax troubles, but it runs, now. Best regards.

simonc commented 10 years ago

Hi @zdravko could you give us some details about the following points? :)

Note that Ruby 1.8 is not supported though ;)

zdravko commented 10 years ago

Hi, @simonc !

SimpleNavigation version 3.13.0 Rails 3.2.18 just a simple menu:

SimpleNavigation::Configuration.run do |navigation| primary.item :patients, 'Label', patients_url(@project) end

Yes, I know 1.8 is not supported... but somehow I have hopes it will do the job.

Best regards.

simonc commented 10 years ago

Ok :) But how do you plan to make SN work on 1.8? Do you patch the Hash syntax or something like that? I don't really know what to say. What renderer to you use?

zdravko commented 10 years ago

Patching hash syntax is easy. Sorry for downgrade. I use html.erb template files. Still searching for where to check things regarding this issue. Really strange. Any hint would be welcome.

Regards, Zdravko.

simonc commented 10 years ago

That's why I was asking for the way you try to use SN. Understanding how you patched it could help me understand what could be going wrong.

Would you be able to show your complete patch diff?

zdravko commented 10 years ago

Simon, hi! This looks terribly wrong what I've done. In item.rb in class Item there is a 'name' method that runs name_generator (bellow) Originally the call contains 'self' argument, like this: SimpleNavigation.config.name_generator.call(@name, self)

I have removed the 'self' part and it appears to be working.

def name(options = {})
  options = { :apply_generator => true }.merge(options)
  if (options[:apply_generator])
    SimpleNavigation.config.name_generator.call(@name)
  else
    @name
  end
end

How much did I mess this up?

Regards, Zdravko

On Tue, 2014-07-29 at 00:15 -0700, Simon Courtois wrote:

That's why I was asking for the way you try to use SN. Understanding how you patched it could help me understand what could be going wrong.

Would you be able to show your complete patch diff?

— Reply to this email directly or view it on GitHub.

simonc commented 10 years ago

I'm sorry @zdravko but without a comprehensive diff of your patches I can't tell you much. The modification you're talking about doesn't say what could be going on :/

zdravko commented 10 years ago

I'll make a full diff, alright. Did you find where I made the change? What is the function of the 'self' argument to the name_generator? I'm way to weak in Ruby to say if this is a good fix. It doesn't look so, it only works.

I didn't make any other patches except syntax one.

Regards, Zdravko ps. I'll need to fork your repository, so I can make a diff, and I can't do that immediately.

On Tue, 2014-07-29 at 03:02 -0700, Simon Courtois wrote:

I'm sorry @zdravko but without a comprehensive diff of your patches I can't tell you much. The modification you're talking about doesn't say what could be going on :/

— Reply to this email directly or view it on GitHub.

simonc commented 10 years ago

The use of self as an argument for name_generator is to provide a way to generate an item's name based on something else than the config-provided name.

If your patch works for you I'll consider the issue closed if you're ok with it :)

zdravko commented 10 years ago

But this self then isn't used in name_generator? Why?

On Tue, 2014-07-29 at 03:31 -0700, Simon Courtois wrote:

The use of self as an argument for name_generator is to provide a way to generate an item's name based on something else than the config-provided name.

If your patch works for you I'll consider the issue closed if you're ok with it :)

— Reply to this email directly or view it on GitHub.

simonc commented 10 years ago

Because it doesn't need to be used by default, the self is provided for more complex uses of name_generator.

simonc commented 10 years ago

You can find some history about this in the following PR https://github.com/codeplant/simple-navigation/pull/144

zdravko commented 10 years ago

Well, then, I can't tell. Please, you decide. Here is my proposal for @name_generator method in Configuration class: def name_generator @name_generator ||= proc { |name, item| name } end I took this from your comments in config file, where proc has two arguments. It works, too. (with the 'self' argument we disscussed before.)

Great gem, though. Thanks. Regards, Zdravko

simonc commented 10 years ago

As long as the patch works for you, it's ok, Maybe it's a matter of blocks arity or something like that.

In any case, nothing will be merged tu support Ruby 1.8. I'm sorry but if you plan on staying with this version you'll have to maintain your own fork of SN :)

I'll close this issue now that you have a working version. Thank you for your feedback ;)