@diosmosis opened this Pull Request on August 30th 2015 Member

This change changes the way menu.js detects the active menu item.

The old way of detecting menu items includes:

  1. Iterating through each <li> element, finding it's associated menu link
  2. Setting the id of the <li> element to something like {{ module}}_{{ action }}
  3. Querying the <li> element w/ the proper ID.

This creates a problem for plugins that add menu items to existing menu tabs (eg, adding a page to Goals). When the page is active, Piwik will look for the menu item for the plugin (eg, MyPlugin) and won't find it or will activate the wrong tab group.

This change fixes this bug by changing the way active items are detected to:

  1. Find all links w/ the right module/action/other parameters.
  2. Getting the closest <li> parents that represent the menu tab / menu item.

Fixes #8624

@diosmosis commented on September 4th 2015 Member

@mattab @tsteur ping-ing for review

@sgiehl commented on September 5th 2015 Member

Just had a small look. It seems not to work when directly opening a specific goal. There is no selection happening in that case. Guess that happens as the goals are in a special "submenu"

@diosmosis commented on September 5th 2015 Member

I tested this specifically since that was the bug I was trying to fix in another plugin. Can you check again? It's possible it doesn't work anymore though...

@sgiehl commented on September 5th 2015 Member

Sure. I'll recheck that...

@sgiehl commented on September 5th 2015 Member

Ah ok. Works! Guess I forgot to kill the assets

@sgiehl commented on September 5th 2015 Member

I've now tried that several more times.
When having only one goal it seems to work always, but as soon as there are some more goals, and the goals submenu is rendered it often doesn't.
Seems to be a timing issue. Invoking the method manually with the correct params, always works.
I'll try to find out why...

@sgiehl commented on September 5th 2015 Member

Guess the reason is, that the dropdowns are not finished rendering. When activateMenu() is called the links found in the menu don't contain the goals. Maybe we need to increase the timeout or maybe also trigger menu activation when the dropdowns are finished

@diosmosis commented on September 5th 2015 Member

Thanks for looking into it! I'll see if I can fix it somehow.

@diosmosis commented on September 7th 2015 Member

When having only one goal it seems to work always, but as soon as there are some more goals, and the goals submenu is rendered it often doesn't.

Can't reproduce this on firefox or chrome. What browser are you using?

@sgiehl commented on September 7th 2015 Member

Chrome on Windows 7
But I can test on Firefox or IE later

@sgiehl commented on September 7th 2015 Member

Retried in Chrome and can't reproduce it there atm. Maybe due to the Chrome update yesterday.
But I did some tests on IE and there the menu doesn't appear on reload:
ie

@diosmosis commented on September 8th 2015 Member

Can reproduce on IE11

@diosmosis commented on September 8th 2015 Member

Only way I could fix this issue was w/ two $timeouts. It should work I think, but it might be too much of a hack...

CC @mattab

@mattab commented on September 9th 2015 Owner

Feedback:

It should work I think, but it might be too much of a hack...

The menu was already rewritten in Piwik 3.0.0 branch so this "hack" is not a problem IMHO

@diosmosis commented on September 9th 2015 Member

When adding a new "Custom dashboard"

Got: no submenu entry was added. (however on page refresh, the submenu entry is displayed as expected.)
Expected: sub menu refreshes and displays the custom dashboard as submenu of Dashboard.

Unrelated bug, happens for me on demo2.

@diosmosis commented on September 9th 2015 Member
@mattab commented on September 9th 2015 Owner

@diosmosis there are couple UI tests failing: http://builds-artifacts.piwik.org/piwik/piwik/8624_module_active_detect/15246/ - could you investigate, and once fixed, +1 to merge

This Pull Request was closed on September 9th 2015
Powered by GitHub Issue Mirror