Skip to content

Conversation

@rise-erpelding
Copy link
Contributor

@rise-erpelding rise-erpelding commented Oct 4, 2023

Description

Makes some minor fixes to the registration and usage of menus:

  • Adds comments to clarify how multiple menus could be used when registering menus and in menu.twig template.
  • Removes unused items variable from menu include - this wasn't hurting anything, but also wasn't needed because we are getting menu items in a different way in the menu template.
  • Changes argument when Timber Menu is instantiated to use correct menu. This also wasn't hurting anything because we only have one menu so the argument is optional, but would become a problem if anyone wanted to add an additional menu to the existing code.

Closes #77

To Validate

  1. Make sure all PR Checks have passed
  2. Pull down this branch
  3. Run npm start
  4. Confirm that this has no negative effects on the way menus currently work: the 'Primary' menu location still exists in the wp-admin and menu displays as before (if your DB does not have a menu, you can use the one from feat: consolidate css #73 ).
  5. Confirm that changes and comments make sense and are appropriate for someone wanting to make updates to menus.

- Adds comments to clarify how multiple menus could be used
- Removes unused items variable from menu include
- Changes Timber Menu argument to use correct menu
@rise-erpelding rise-erpelding marked this pull request as ready for review October 4, 2023 19:48
Copy link
Contributor

@dustin-jw dustin-jw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are all good improvements, and I think future developers would have a reasonably easy time figuring out how to add more menus as needed.

@dustin-jw dustin-jw merged commit c55c5bc into main Oct 9, 2023
@dustin-jw dustin-jw deleted the fix--improve-menu branch October 9, 2023 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve menu usage

3 participants