- Issue created by @alison
- 🇮🇳India dev2.addweb
nilesh.addweb → made their first commit to this issue’s fork.
- Merge request !7Add support to restrict BigMenu usage to selected menus. → (Open) created by Unnamed author
- Status changed to Needs review
7 months ago 5:41am 19 September 2024 - 🇮🇳India dev2.addweb
Hi, I have added config field to allow certain menus and created MR. Please review it.
- 🇺🇸United States alison
@nilesh.addweb Thank you! When I apply the patch, though, I no longer have Big Menu functionality on any of my menus -- even after enabling those menus in the Big Menu settings form. I'm not sure why, I don't see any errors in logs, it's just not functioning for me -- did it work for you?
Besides that, a few other notes:
- The first time I go to the settings form, I get a warning in logs (⬇️). I think it'll be happier if you change the default value (in bigmenu.settings.yml) to
{ }
.
Warning: foreach() argument must be of type array|object, null given in Drupal\bigmenu\BigMenuForm->buildOverviewForm() (line 62 of /app/web/modules/contrib/bigmenu/src/BigMenuForm.php)
- As the patch is currently written, Big Menu is immediately "disabled" on all menus -- site admins have to go to the settings screen and enable menus, in order for Big Menu to keep working.
- Either there should be an update hook to enable Big Menu on all menus; or,
- The setting could be revised so that this option is a "negative" option, i.e. checking a box next to a menu would disable it (example: "disable_themes" option in the Chosen module, but this would be menus, of course) (this field in the Chosen module settings is also an example of the checkbox field type); or
- Allow either way: list the menus with checkboxes next to each one, and then have a "negate this condition" option, like in Block visibility settings. (This option would also need an update hook.)
- (what I'm referring to) In Block visibility settings > Vocabulary, there's a "negate this condition" checkbox.
- I think checkboxes would be better UX for this field, and more consistent with similar fields on Drupal admin forms (instead of a select field).
- I think the language would be better as "enabled menus" rather than "available menus", what do you think? (Or "disabled," if you go with the "negative" option.
- The first time I go to the settings form, I get a warning in logs (⬇️). I think it'll be happier if you change the default value (in bigmenu.settings.yml) to
- 🇮🇳India dev2.addweb
Hi @alison, Thanks for your suggestion, I’ve made some changes based on #5,
1. I didn’t encounter any warnings when changing to default value.
2.I’m not entirely sure what you meant regarding flow, but as of now, current flow is enable module and set settings as per need and you can see the big menus effect on configured menus.
All other suggested changes have been implemented, Please review it.