codeplant/simple-navigation

When using local variables, all the right menu items contain the same css classes

vill opened this issue · 4 comments

vill commented

Hi there.

Environment

  • Ruby 2.4.0
  • Rails 5.2.0
  • Simple Navigation 4.0.5

Current behavior

# frozen_string_literal: true

# Bootstrap v4 Navbar https://getbootstrap.com/docs/4.0/components/navbar/
SimpleNavigation::Configuration.run do |navigation|
  navigation.selected_class        = ''
  navigation.active_leaf_class     = 'active'
  navigation.autogenerate_item_ids = false

  navigation.items do |primary|
    primary.dom_class = 'navbar-nav ml-auto'

    if user_signed_in?
      # ...
    else
      primary.item :registration, 'Sign up', new_user_registration_path,
                   link_html: { class: 'nav-link' }, html: { class: 'nav-item' }
      primary.item :session, 'Log in', new_user_session_path,
                   link_html: { class: 'nav-link' }, html: { class: 'nav-item' }
    end
  end
end

The HTML structure looks correct for the menu item 'Sign up':

<ul class="navbar-nav ml-auto">
  <li class="nav-item active">
    <a class="nav-link" href="/user/sign_up">Sign up</a>
  </li>
  <li class="nav-item">
    <a class="nav-link" href="/user/sign_in">Log in</a>
  </li>
</ul>

The same config when using a local variable with common parameters:

# frozen_string_literal: true

# Bootstrap v4 Navbar https://getbootstrap.com/docs/4.0/components/navbar/
SimpleNavigation::Configuration.run do |navigation|
  navigation.selected_class        = ''
  navigation.active_leaf_class     = 'active'
  navigation.autogenerate_item_ids = false

  navigation.items do |primary|
    primary.dom_class = 'navbar-nav ml-auto'
    html_options      = { link_html: { class: 'nav-link' }, html: { class: 'nav-item' } }

    if user_signed_in?
      # ...
    else
      primary.item :registration, 'Sign up', new_user_registration_path, html_options
      primary.item :session, 'Log in', new_user_session_path, html_options

      # or use double splat operator
      # primary.item :registration, 'Sign up', new_user_registration_path, **html_options
      # primary.item :session, 'Log in', new_user_session_path, **html_options
    end
  end
end

when going to the menu item 'Sign up':

<ul class="navbar-nav ml-auto">
  <li class="nav-item active">
    <a class="nav-link" href="/user/sign_up">Sign up</a>
  </li>
  <li class="nav-item active">
    <a class="nav-link" href="/user/sign_in">Log in</a>
  </li>
</ul>

Expected behavior

When switching to the menu item 'Sign Up', the 'Sign In' item should not have the css classes from navigation.active_leaf_class and navigation.selected_class (in the example it is equal to an empty string '').

andi commented

So when you inline the html_options variable, it works correctly?

vill commented

Works correctly only so:

# frozen_string_literal: true

# Bootstrap v4 Navbar https://getbootstrap.com/docs/4.0/components/navbar/
SimpleNavigation::Configuration.run do |navigation|
  navigation.selected_class        = ''
  navigation.active_leaf_class     = 'active'
  navigation.autogenerate_item_ids = false

  navigation.items do |primary|
    primary.dom_class = 'navbar-nav ml-auto'

    if user_signed_in?
      # ...
    else
      primary.item :registration, 'Sign up', new_user_registration_path,
                   link_html: { class: 'nav-link' }, html: { class: 'nav-item' }
      primary.item :session, 'Log in', new_user_session_path,
                   link_html: { class: 'nav-link' }, html: { class: 'nav-item' }
    end
  end
end
andi commented

I suspect that if you pass the variable, the object options reference passed to primary.item is the same for both items, i.e. if under the hood the "active" flag is applied to one item, it is also valid for the second item. My guess is that if you have 3 items of which 2 use the variable and the last one uses an inline hash, the first 2 would be active and the 3rd would be ok... just a guess.

vill commented

My mistake. I have to use the deep_dup method for a nested hash:

# frozen_string_literal: true

# Bootstrap v4 Navbar https://getbootstrap.com/docs/4.0/components/navbar/
SimpleNavigation::Configuration.run do |navigation|
  navigation.selected_class        = ''
  navigation.active_leaf_class     = 'active'
  navigation.autogenerate_item_ids = false

  navigation.items do |primary|
    primary.dom_class = 'navbar-nav ml-auto'
    html_options      = { link_html: { class: 'nav-link' }, html: { class: 'nav-item' } }

    if user_signed_in?
      # ...
    else
      primary.item :registration, 'Sign up', new_user_registration_path, html_options.deep_dup
      primary.item :session, 'Log in', new_user_session_path, html_options.deep_dup
    end
  end
end