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
Refactoring the menu classes and adding top menu hookable #1403
Comments
The menu refactoring is pure 'php' refactoring to ensure all menus (main analytics menu, top bar menu and admin menu) provide the same features (ordering, etc.). I think this can be done independently of the UI refactoring and other menu improvements. |
Ok. Very good. Then wrtnaming convention, please use Piwik_Menu_Abstract (instead of Piwik_Abstract_Menu). At the same time I think we should move core/PluginsFunctions/Menu.php and core/PluginsFunctions/AdminMenu.php into core/Menu/ |
Agreed. For consistency I should also rename Piwik_Menu to Piwik_Menu_Menu and Piwik_AdminMenu to Piwik_Menu_AdminMenu, does that sound reasonable? Another thing to consider is renaming Piwik_Menu to Piwik_Menu_MainMenu (as we have TopMenu and AdminMenu), but this would definetely break backwards compatibility with 3rd party plugins (because of the hook Menu.add -> MainMenu.add and Piwik_AddMenu -> Piwik_AddMainMenu). I guess this should be left untouched. |
I would say Piwik_Menu_Main, Piwik_Menu_Admin and Piwik_Menu_Top, and of course leaving the existing functions unchanged as we need to keep backward compatibility. |
Alright. I'd like a code review of my menu restructuring:
Note:
I need suggestions for:
would produce ?module=A&action=B#C=Foo
Snapshot of Alerts plugin coming soon. |
It seems the feedback link doesn't add a fragment, but a HTML id attribute: We shouldnt add complicated logic just to handle this specific use case. I think the generated HTML for the top bar would contain some sort of ID that you could use and select instead of topbar-feedback in https://github.com/piwik/piwik/blob/master/plugins/Feedback/templates/feedback.js#L2
Yes that sounds good (the $view->topBar = Piwik_GetTopMenu() could then be refactored in setGeneralVariablesView maybe?
because it is currently only used in the top bar, you can maybe overwrite add() in the Top Menu class to allow for HTML as a menu entry. |
Patch updated.
to disable the TopMenu which is now set by default in setGeneralVariablesView(). |
looks good! Few questions
I also see the same call twice in plugins/Goals/Goals.php and Referers.php
Appart from these minor things, it looks good - please confirm that you have tested the code, submit last patch and I will commit it, thanks! |
I tested all code intensively! - However, I just discovered a nasty bug for which I have no good idea how to fix it. For the "Visits" page the submenus are added by different plugins. As a result the first level menu "Visits" gets added indirectly (as a consequence of adding a submenu). This leads to the URL of UserSettings.index being the one that is associated with the first level menu. A later call to
gets ignored, as the menu has already been added. The quick-fix in this case would be a call to
this however would mean, that we'd need to do this for every first level menu entry, as new plugins could add submenus to them before the first level menu has been added. Alternative: Allow to directly overwrite menu entries (would make Piwik_Edit*Url) obsolete. Give me a short response / idea and I adjust it accordingly. |
Let me know when bug is fixed, and I'll commit |
All fixed. Ready to commit :) |
I applied the patch but the top menu didn't display, and the menu was displaying in random orders (see screenshot). is it working for you? |
Attachment: menus in random orders, top menu missing except API |
Also you can put the Piwik header files to the new files (eg. see core/Piwik.php header). Also can you can remove empty comments (or write comments ;) from core/Menu/Abstract.php and other files. |
The ordering works perfectly for me, did you maybe had conflicts in the files? When I updated I had several conflicts that I had to resolve. I upload a new patch containing comments, correct header and diffed against the latest trunk revision (r2400). |
the patch is missing Actions/Actions.php menu calls with weights it seems. cheers |
Also when I visit the tracker after applying patch, I get the error: The menu files should not be loaded in the tracker as it slightly impacts performance. |
Actions.php - Yeah :( My IDE screwed up the SVN states and therefore did not include the file in the patch. ExampleUI - Added ordering. Added comments to the Piwik_Add*Menu functions. I agree that the menu should not be loaded when running the tracker - This is a pre-existing condition though as the menu is/was being loaded in core/PluginManager.php. The error you get is due to the autoloader being unavailable in tracking mode. To avoid the error I manually included the Abstract.php in PluginManager.php for now. I applied the patch on a fresh checkout and it worked for me - Please recheck it though my IDE might've missed some files again. |
Attachment: Patch against latest |
(In [2409]) Fixes #1403 Patch by halfdan: great stuff! Menu code is now clean and refactored. |
The "Alerts plugin" is not fixed yet of course, reopening. halfdan, do you know when a beta will be available for Alerts? cheers |
Regression: It seems the feedback link doesn't work anymore in trunk. It loads the content instead of opening a popover. |
(In [2412]) refs #1403 feedback form is working again |
SteveG: Thanks, missed that file in the patch. Thanks for commiting the refactored menu! |
(In [2514]) Refs #1403 |
I'm currently writing a new plugin for Piwik. It will implement a functionality similar to Google Analytics Alerts. Users can create custom alerts "Goal revenue increased by 10%", "Unique visitors greater than 1000", etc. of which they will then get notified of if they occur. An Alert can be set for 1:n websites.
ToDo:
The abstract class would contain the basic logic of a menu. Piwik_Menu e.g. overrides applyOrdering() to position the Overview and Evolution entries at the first position. (seperate patch)
I opened this ticket for discussion. As soon as I'm having a first preview I'll upload it here for review and comments.
The text was updated successfully, but these errors were encountered: