Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rewrite the way menu.js detects the active menu item (don't set elements, look for links w/ the right URL) #8669

Merged
merged 4 commits into from Sep 9, 2015
Merged

Rewrite the way menu.js detects the active menu item (don't set elements, look for links w/ the right URL) #8669

merged 4 commits into from Sep 9, 2015

Conversation

diosmosis
Copy link
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 diosmosis added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Aug 30, 2015
@diosmosis diosmosis added this to the 2.15.0 milestone Aug 30, 2015
@diosmosis
Copy link
Member Author

@mattab @tsteur ping-ing for review

@sgiehl
Copy link
Member

sgiehl commented Sep 5, 2015

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
Copy link
Member Author

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
Copy link
Member

sgiehl commented Sep 5, 2015

Sure. I'll recheck that...

@sgiehl
Copy link
Member

sgiehl commented Sep 5, 2015

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

@sgiehl
Copy link
Member

sgiehl commented Sep 5, 2015

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
Copy link
Member

sgiehl commented Sep 5, 2015

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
Copy link
Member Author

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

@diosmosis
Copy link
Member Author

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
Copy link
Member

sgiehl commented Sep 7, 2015 via email

@sgiehl
Copy link
Member

sgiehl commented Sep 7, 2015

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

Refs #8624, do not set the ID of link elements in the reporting menu …
8df498e 
…and query the IDs when detecting the active menu item, instead find the link w/ the appropriate parameters get the parent li element. Also, do not activate the menu item until all angular menu group dropdowns are rendered.
@diosmosis
Copy link
Member Author

Can reproduce on IE11

@diosmosis
Copy link
Member Author

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
Copy link
Member

mattab commented Sep 9, 2015

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
Copy link
Member Author

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.

Do not use indexOf to check if a query string has a param, instead us…
7c4e103 
…e broadcast to get the actual value in the hash/url.
@diosmosis
Copy link
Member Author

Going to http://localhost/piwik-master/index.php?module=CoreHome&action=index&idSite=1&period=day&date=yesterday#/module=Referrers&action=index&idSite=1&period=day&date=yesterday

Should be fixed.

@mattab
Copy link
Member

mattab commented Sep 9, 2015

@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

diosmosis added a commit that referenced this pull request Sep 9, 2015
@diosmosis
Merge pull request #8669 from piwik/8624_module_active_detect
f7678c9 
Fixes #8624, rewrite the way menu.js detects the active menu item (don't set elements, look for links w/ the right URL)
@diosmosis diosmosis merged commit f7678c9 into master Sep 9, 2015
@diosmosis diosmosis deleted the 8624_module_active_detect branch September 9, 2015 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
2.15.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@diosmosis @sgiehl @mattab