-
Notifications
You must be signed in to change notification settings - Fork 381
Refactor header bar and footer menu to be configurable section plugins #5002
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
base: dev
Are you sure you want to change the base?
Conversation
- Also adds a description option to the menu button component - Header menu template supports a `below` key for creating dropdown menus
demiankatz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @aleksip -- here is a partial review, primarily focused on the documentation in the configuration files. I thought it might be best to finish this discussion and make sure the docs are correct before I check the implementation more carefully.
|
@demiankatz Many of the keys documented in the configuration files were copied over from existing plugins and not actually used at all. Supported keys depend on a combination of plugin PHP code and HTML template code. I have now tried to check that only the keys actually currently used are documented. |
demiankatz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @aleksip -- a few quick thoughts in response to the latest changes. I'm off to meetings for most of the day now, but I'll review the rest as time permits!
config/vufind/AdminMenu.yaml
Outdated
| # - iconMethod: method to dynamically create the icon name; ignored when | ||
| # icon is explicitly set. | ||
| # - checkMethod: the name of an AdminMenu navigation plugin method to perform | ||
| # - checkMethod: the name of an AdminMenu section plugin method to perform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth a separate PR to fix the AccountMenu/AdminMenu docs, since that's not directly related to the rest of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you rolled out those changes but haven't opened a separate PR yet. Is that on your to-do list? Do you need me to do anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have now opened a separate PR: #5012. Probably best to merge it only after this and the third section follow-up PRs have been merged.
config/vufind/FooterMenu.yaml
Outdated
| @@ -0,0 +1,71 @@ | |||
| # If this file is empty or missing, default settings will be used, matching | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth adding a "purpose statement" first line to all of these files, like:
# This file configures the Footer section plugin, which renders menus at the bottom of every page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wonder if we should call this FooterMenu, since it's really a menu in the footer and not the whole footer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the rename. What do you think about the earlier suggestion about adding a first-line comment explaining the context/purpose of the section to each of these files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a good idea. Did not have time to do it before, but have added them now.
config/vufind/HeaderBar.yaml
Outdated
| @@ -0,0 +1,48 @@ | |||
| # If this file is empty or missing, default settings will be used, matching | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we rename Footer to FooterMenu, I'm not exactly sure what to call this. It's also not exactly the header in its entirety, but it's more than just a menu....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the functionality it contains, instead of just links to pages? It does configure items in a single navbar though. So calling it HeaderMenu would perhaps not be horribly wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current header.phtml does contain the searchbox, but the section helper call could be moved from layout.html to header.phtml?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, my thought was that the FooterMenu is very much a menu with a specific shape... but the Header section is much more open-ended, with multiple sub-templates that can all do different things. Something like HeaderBar might be fine, though -- I just don't think its exactly a Menu.
I would definitely favor moving the section helper calls inside header.phtml and footer.phtml rather than going directly to layout.phtml, because in many cases (certainly in real practice for several sites at Villanova), the entire files may be overridden with customizations that completely replace the default functionality. Running the extra configuration processing, etc., only to render a totally unrelated template seems counterproductive.
|
@demiankatz Just noticed that e.g. if the third footer column is removed in |
demiankatz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @aleksip, I've done a more thorough review of the code. I haven't done any hands-on testing yet, but I thought getting through the documentation and technical review first would be a good place to start. I'll actually try things out once we've finished this part of the conversation.
config/vufind/FooterMenu.yaml
Outdated
| # a check whether to show the group. If omitted, group will always display. | ||
| # | ||
| # Array keys for every menu item could be: | ||
| # - name: name of an item |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is name used for? We should probably explain why you would use/want this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was left over from a copy-paste, now removed.
config/vufind/FooterMenu.yaml
Outdated
| # Array keys for every menu item could be: | ||
| # - name: name of an item | ||
| # - label: the text shown as link, will be translated - required | ||
| # - route: route name used to generate link target - route or url required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might make it slightly more clear that we're talking about which settings are required, rather than the content of this setting.
| # - route: route name used to generate link target - route or url required | |
| # - route: route name used to generate link target - required unless url is set |
config/vufind/FooterMenu.yaml
Outdated
| # - label: the text shown as link, will be translated - required | ||
| # - route: route name used to generate link target - route or url required | ||
| # - routeParams: parameters for dynamic routes | ||
| # - url: target url, alternative to route - route or url required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # - url: target url, alternative to route - route or url required | |
| # - url: target url, alternative to route - required unless route is set |
config/vufind/HeaderBar.yaml
Outdated
| @@ -0,0 +1,53 @@ | |||
| # This file configures the HeaderBar section plugin, which renders menus at the | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be more accurate? Or maybe "menus and other controls"?
| # This file configures the HeaderBar section plugin, which renders menus at the | |
| # This file configures the HeaderBar section plugin, which renders controls at the |
config/vufind/HeaderBar.yaml
Outdated
| # Array keys for every menu item could be: | ||
| # - name: name of an item | ||
| # - label: the text shown as link, will be translated - required | ||
| # - route: route name used to generate link target - route, url or template required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # - route: route name used to generate link target - route, url or template required | |
| # - route: route name used to generate link target - required unless url or template is set |
| $menu = $this->getFooterMenu( | ||
| $container, | ||
| FooterMenu::getDefaultMenuConfig(), | ||
| $this->getFooterMenuCheckMethods(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this approach of mocking out all the check methods prevent us from covering most of the logic in tests? Do we also need tests covering the unmocked class to confirm that constructor inputs lead to expected check method outputs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now refactored the new tests to not use a mocked class at all. I guess the same thing or something similar should be done to the existing AccountMenu and AdminMenu tests. Should I do those changes in #5012 or in yet another follow-up PR? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to add to #5012 -- all menu-related improvements in one PR is fine with me. :-)
| public function testDefaultMenuAllCheckMethodsReturnFalse() | ||
| { | ||
| $container = $this->getContainerWithSectionRelatedServices(); | ||
| $menu = $this->getHeaderBar( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question about mocking also applies here.
| color: var(--#{$prefix}link-hover-color); | ||
| } | ||
|
|
||
| button, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this styling impact existing mechanisms, or is this just for the new submenus? (I just want to figure out what needs testing, and also make sure we're not creating a new kind of menu that conflicts with existing menus already in the system).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rule is meant to affect the menu-button component wherever it is used. I have tried to make the rule very specific so as to not affect anything else. It should only style the newly added description element which did not exist previously.
| 'label' => $belowItem['label'], | ||
| 'url' => $url, | ||
| 'description' => $belowItem['description'] ?? false, | ||
| 'selected' => $_SERVER['REQUEST_URI'] === $url, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to do this without referencing a superglobal in a template? That's not generally desirable!
| return [ | ||
| 'label' => $this->displayLanguageOption($langName), | ||
| 'url' => $this->url()->addQueryParameters(['lng' => $langCode]), | ||
| 'selected' => $this->layout()->userLang == $langCode, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an opportunity here to pass these language settings from the HeaderBar plugin instead of through the layout? Or might we actually need a language-setting-specific view helper?
Again, I don't insist on changing this now, but I'm just thinking in the longer term -- using global layout variables should really only be an absolute last resort, and it feels like there may be an opportunity to more clearly encapsulate these settings.
If I'm reading the YAML loading code correctly, the easiest solution might be to set either the |
Thank you, this seems to work just fine. I have added a note about the possibility, as I suspect this might be a common requirement. Probably good to add the note to the files in #5012 too? |
Follow-up to #4796