-
Notifications
You must be signed in to change notification settings - Fork 1
Description
To this day, we are still basing our views off of the tabs Icinga Web provides since the initial release. Nothing has changed, no one took the opportunity to rewrite them from scratch. As such, all of the limitations from the past still apply, which is a shame:
- Title Tab text has no max limit icingaweb2#5384
- Adapt pane controls to available width with dropdown icingaweb2#3787
- Searchbar autosubmit doesn't update extension urls icingadb-web#373
ipl-web/src/Compat/CompatController.php
Lines 352 to 361 in 8b5b987
// As long as we still depend on the legacy tab implementation // there is no other way to influence what the tab extensions // use as url. (https://github.com/Icinga/icingadb-web/issues/373) $oldPathInfo = $this->getRequest()->getPathInfo(); $oldQuery = $_SERVER['QUERY_STRING']; $this->getRequest()->setPathInfo('/' . $redirectUrl->getPath()); $_SERVER['QUERY_STRING'] = $redirectUrl->getQueryString(); $this->tabs->ensureAssembled(); $this->getRequest()->setPathInfo($oldPathInfo); $_SERVER['QUERY_STRING'] = $oldQuery; - Insufficient extensibility (https://github.com/Icinga/icingaweb2/blob/70f29827f921f24b2afc0556c4800dfa55a2d0c1/application/controllers/IframeController.php#L91-L101)
Let's do it, finally!
Considerations
Backwards compatibility is key here. This isn't just about PHP but also about CSS. (JS not so much)
PHP
The current implementation ipl\Web\Widget\Tabs cannot be re-used. I'd like to introduce ipl\Web\Layout\TabBar instead. They're part of the main layout after all.
This brings me to class ipl\Web\Layout\Controls which is responsible to render the tabs right now. I don't want to change that, but we have to make sure any implementation relying on the methods getTabs(): ipl\Web\Widget\Tabs and setTabs(ipl\Web\Widget\Tabs): static still work and get the legacy tabs.
This is also the reason why I'm proposing the name TabBar instead of Tabs, as the latter is already taken. (It's also a fitting name IMHO as there's already an ActionBar (ipl\Web\Widget\ActionBar))
So ipl\Web\Layout\Controls needs getTabBar(): TabBar and setTabBar(TabBar): static.
Next is ipl\Web\Compat\CompatController. It also needs to support both implementations in parallel. But only one must be rendered of course. That is:
A controller populating the tab bar with the new method ipl\Web\Compat\CompatController->assembleTabBar(TabBar): void enables the new implementation by doing so. If this method is not overridden, but ->getTabs(): ipl\Web\Widget\Tabs is, the legacy implementation is used instead. If neither of both are populated, the tab bar is rendered by default.
CSS
It might be tempting to re-use some CSS classes such as primary-nav or nav, but I believe we should only rely on entirely new rules. They don't seem to control that much either so let's ignore them.
The same applies to tabs. Themes may reference this but honestly, a theme should only depend on variables and this is the only part I'd preserve for compatibility. Color variables referenced by icingaweb2/public/css/icinga/tabs.less should be respected by the new rules, but only as a fallback. For this to work, we'd have to use the following variables:
@menu-bg-color@menu-color@tab-hover-bg-color
The following ones can be replaced, as Icinga Web already maps them correctly:
@body-bg-color->@default-bg@text-color->@default-text-color
Visibility of the tab bar is also important, or rather, its invisibility. 😆 Using ?showCompact or exporting a view to PDF hides the old tabs. This must still be the case for the tab bar, but usually this is taken care by hiding the entire .controls section. So no special handling required here, probably.
Implementation
Now that compatibility off the table, let's look at the implementation in detail. We should start with basic functionality first, but keep future additions in mind, especially those I already listed at the beginning. (e.g., Auto collapsing of too many items)
ipl\Web\Layout\TabBar
This class should be based on the following description:
- It's still a list, but probably an ordered list, since the tabs contained have a specific order
- CSS should apply flexbox layout
- Min and max width of items must be declared, with appropriate overflow handling
- Items can be added with methods
->addTab(string, ipl\Web\Widget\Link): staticand->addToDropdown(string, ipl\Web\Widget\Link): static ->setActive(string): staticallows to set a specific item active, by default the first registered item is active->get(string): ipl\Web\Widget\Linkand->getActive(): ipl\Web\Widget\Linkcan be used to access items- Unless something has been added to the dropdown, the dropdown is not rendered
- The refresh item is always there and must not be enabled nor must be a specific url be registered. The url of the active item must be used automatically as refresh url
- The close button is no actual item and only rendered by JS
- This requires tight integration with Icinga Web as this relies on handling for the event
layout-change - An Icinga Web behavior provided by default should facilitate this
- This requires tight integration with Icinga Web as this relies on handling for the event
- Extensions might still be a thing or at least I think we need something to differentiate between items that lead to actual new content and items that only transform or pipe the current content to somewhere else
- Take the extensions to add a new dashboard or menu item for example, they expect the current item's url being passed to them. This isn't so far off of what the refresh item will do so why not make this a public functionality?
- This should be specific to items added to the dropdown. Maybe by another parameter to
->addToDropdown(…, ?callable … = null): staticthat allows to pass a function in order to adjust the url based on the item that's active
Extending or adjusting actual items of the tab bar, the li's, should not be required. Most of the time, previous cases where this was required were about the current item's url. Since the refresh button should now automatically use the active item's url, I think that solves the case of Icinga Web's IframeController elegantly, in case the entire ipl\Web\Widget\Link is being used, not just it's URL.
Javascript
As noted above, automatic collapsing of too many items should not be part of the first iteration, but to render the close button conditionally we need JS anyway. This should be handled by a behavior provided here by default for Icinga Web. Icinga/icingaweb2#5238 should come in handy here.