MVC: add support for pluggable dynamic menu items and move some existing parts out of the MenuSystem class#10113
Conversation
|
|
||
| abstract class MenuContainer | ||
| { | ||
| private ?MenuSystem $menusystem = null; |
There was a problem hiding this comment.
Given this is private, how will the favorites code access getItem() to build the breadcrumb labels for the Favorites menu?
There was a problem hiding this comment.
@Greelan see appendItem() examples in Firewall and Interfaces
There was a problem hiding this comment.
Thanks, I've looked at those. I think the favorites scenario is different, because the labels can't just be read from the config, instead they need to be built from the existing menu tree (eg a favorite pointing to /ui/firewall/filter should show as Firewall: Rules [new]). Perhaps I'm missing something?
There was a problem hiding this comment.
It might need some additional logic to prevent the favorite item needing the same uri as the actual target, but we'll see it when we'll get there. One of the options might be to just store a (md5) hash of the link and reflect the click action for example.
The favorite description itself could be part of the stored content to prevent the need to render the favorites if everything else is already there (since other dynamic content may have been set to favorite as well).
There was a problem hiding this comment.
Agree it will need further thinking down the track. I wanted to avoid storing the label in case it became stale (menu item breadcrumbs changed).
Saying that though made me realise that there is a similar issue - though less likely - for the endpoint itself, eg when a MVC migration occurs. It's a bit different since the pruning logic will clean it up, but the consequence is that the menu item will be un-favourited. Not fatal but not ideal from a UX perspective.
There was a problem hiding this comment.
Thanks! I will have a closer look at it when I get a chance.
There was a problem hiding this comment.
OK, I've now spent some time with this and have done some re-work to use the pluggable system, link classes, and JS wrapper. Will need to wait until the next OPNsense release for testing as it is too challenging to patch an existing installation with the needed upstream commits, given they build on other commits as well
There was a problem hiding this comment.
Just FYI I don't think this will land in 26.1.7 next week but 26.1.8 is very likely.
…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.
AdSchellevis
force-pushed
the
mvc_menu_pluggable
branch
from
df34601 to
f4ee4d2
Compare
…to be used for favorites.
This commit offers a couple of things, first of all it adds a unique class name on every traversable menu item so we can easily use javascript to pass a click event, for example:
$(".menu_ref_7e46272fe380827861cbaf5b484c43c9")[0].click()
We need this in order to link an item, but not use its actual uri in our href later, as this would confuse the "active" item selected.
Next, we offer the option to inject an additional link class ( e.g. $this->appendItem(..,..,['linkclass' => 'my_link_class']); ), as this offers a way to register on the "favorite" buttons.
Finally we cleanup the menu code a bit so the volt and legacy template contain less logic.
fichtner
left a comment
fichtner
left a comment
There was a problem hiding this comment.
Does LinkClass also work for the migration assistant icon?
Nope, it's a class on the link, not on its content. I was doubting refactoring the whole menu, but since I wanted to limit the impact, only pushed the concepts in there that are currently missing. Eventually it would be practical to render this from Javascript and offer an endpoint which contains all relevant items, but that's quite some work I'm afraid. |
3 participants
In most cases we use static menu registrations, 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:
Always try to minimize the amount of code inside these plugins as this code will be executed on each page load.
To explain the concept, both interface and firewall menu items have been moved to their new classes in this PR.