Conversation
Greelan
force-pushed
the
favorites-refactor
branch
from
35e4431 to
c50a78a
Compare
|
|
||
| if (!empty($username)) { | ||
| try { | ||
| $cfg = Config::getInstance(); |
There was a problem hiding this comment.
We should try to avoid direct config access, I noticed the dashboard had a similar issue, so I just refactored that in 3c3f2c4
When we build the dashboard, the User model didn't exist yet, but is backwards compatible.
There was a problem hiding this comment.
OK, I've refactored to mirror
|
|
||
| // per-user favorites - generating requires getItems() call | ||
| $favorites = new \OPNsense\Core\Favorites($_SESSION['Username'] ?? ''); | ||
| $this->view->menuFavorites = $favorites->buildFavoritesEntries($this->view->menuSystem); |
There was a problem hiding this comment.
We should be able to inject entries in a similar way as Interfaces are being injected, but it might be better to refactor some code first there as we also have the issue of menu items being conditionally being registered for isc-dhcp.
|
@Greelan don't mind my comments too much, I'm reviewing your PR a bit and leaving some pointers of things we should look at, no direct action is required. |
No problem, I've made some conforming changes to address the first comment, so hopefully that helps |
|
@Greelan it will take some time to ingest this as I'm mostly working on this in my free time and it does need some changes in our code to ease the proces as well, but I do like your idea, the code just needs some work on both ends (the feature and hooks to inject this) |
|
Absolutely fine, I was already conscious that this is lower priority given it is not on the roadmap, and you and others have plenty of other things to do. My commits and comments aren't intended to be pushy xD |
|
@AdSchellevis no doubt this was your intent in part, but I assume that I could use the new |
|
@Greelan I'm afraid we can't do this in the frontend as we need to modify the menu as well, let me think on this for a while, it's still on the radar, just busy :) |
|
All good, my thinking was just around rendering of the favorites entries and the star icons |
…ing parts out of the MenuSystem class In most cases we use static menu registartions, but there are exceptions which depend on interfaces for example. While looking at #10033, a longer standing wish came up again, which is the reason to add this support right now. It also helps in removing some legacy components for good via plugins. To register new menu items, the following pattern may be used: * In your model, derive a Menu class from MenuContainer * implement a method collect() which should add new menu items via the appendItem() {bound to appendItem in MenuSystem} Always try to minimize the amount of code inside these plugins as this code will be executed on each page load.
…ing parts out of the MenuSystem class In most cases we use static menu registartions, but there are exceptions which depend on interfaces for example. While looking at #10033, a longer standing wish came up again, which is the reason to add this support right now. It also helps in removing some legacy components for good via plugins. To register new menu items, the following pattern may be used: * In your model, derive a Menu class from MenuContainer * implement a method collect() which should add new menu items via the appendItem() {bound to appendItem in MenuSystem} Always try to minimize the amount of code inside these plugins as this code will be executed on each page load.
Greelan
force-pushed
the
favorites-refactor
branch
from
6580299 to
055dff6
Compare
2 participants
Implements a Favorites section:
Some sample screenshots attached.
Supersedes #10028
Depends on 1c2405e