Twig component: Make navigation bar management independent #33702
Conversation
M0rgan01
commented
Aug 23, 2023
M0rgan01
commented
| Questions | Answers |
|---|---|
| Branch? | develop |
| Description? | Make navigation bar management independent |
| Type? | improvement |
| Category? | BO |
| BC breaks? | no |
| Deprecations? | no |
| How to test? | CI 馃煝 and auto tests 馃煝 |
| Fixed issue or discussion? | Fixes #33701 |
| Related PRs | - |
| Sponsor company | - |
M0rgan01
force-pushed
the
improvement/33604
branch
from
f8e566d
to
fd922eb
Compare
M0rgan01
force-pushed
the
improvement/33604
branch
from
fd922eb
to
b7baf33
Compare
M0rgan01
force-pushed
the
improvement/33604
branch
from
b7baf33
to
9a1b256
Compare
M0rgan01
force-pushed
the
improvement/33604
branch
from
9a1b256
to
bc4509c
Compare
There was a problem hiding this comment.
Thanks @M0rgan01
Only a few suggestions, but nothing mandatory as we'll have to refacto some of the classes at some point anyway
| public int $currentTabLevel; | ||
| public array $tabs; | ||
| public int $currentTabLevel = 0; | ||
| public array $navigationTabs = []; |
There was a problem hiding this comment.
It's ok for this one but I noticed you and maybe @boherm as well kept a lot of public fields in the component classes in previous PRs
We shouldn't have public properties on these components except for variables that we absolutely have to inject/override in the component So mostly only variables that will come from twig and that the controller injected I guess
All the other public properties should be transformed in getter functions, and ideally they should only be populated Just In Time in the getter itself Except for "advanced" variables that depend on each other in which case the mount approach may be more adapted
| /** | ||
| * @return array<int, MenuLink> | ||
| */ | ||
| public function buildNavigationTabs(Tab $tab): array |
There was a problem hiding this comment.
| public function buildNavigationTabs(Tab $tab): array | |
| public function getNavigationTabs(Tab $tab): array |
There was a problem hiding this comment.
Another suggestion/refacto we'll have to handle in this service I think We don't cache the result so we'll have to think about how we can optimize this service to avoid doing the same DB queries multiple times for multiple components (not in this PR 馃槈)
There was a problem hiding this comment.
BTW I love this idea of getting the siblings this way instead of the previous quadruple loop 馃槄 This is much smarter this way
| /* @var $currentLevelTab Tab */ | ||
| foreach ($currentLevelTabs as $currentLevelTab) { | ||
| $tabLang = $currentLevelTab->getTabLangByLanguageId($this->getContextLanguageId()); | ||
| $menuLink = new MenuLink( |
There was a problem hiding this comment.
This MenuLink DTO was created as a very simple one, maybe we'll have to improve it (add fields that are used everywhere) Or create alternative ones like a dedicated TabLink DTO or something similar (not in this PR as well)
There was a problem hiding this comment.
I agree, might as well create a new TabLink
ps-jarvis
added
the
Waiting for QA
florine2623
added
QA 鉁旓笍
jolelievre
changed the title
