Skip to content
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

Remove Roots_Nav_Walker as default for all menu #1217

Merged
merged 1 commit into from
Dec 2, 2014
Merged

Remove Roots_Nav_Walker as default for all menu #1217

merged 1 commit into from
Dec 2, 2014

Conversation

leopuleo
Copy link
Contributor

@leopuleo leopuleo commented Dec 1, 2014

I love how Roots_Nav_Walker() create menu html.
But, I believe that apply by default to all menus is too much.

Often I use the menu widget to show complex menus with sub-element.
In these situations sub-elements are wrapped in dropdown menus, which is fine for primary menu but not for aside or footer.

I suggest to:

  • remove Roots_Nav_Walker() as default for all menus
  • add Roots_Nav_Walker() to Primary menu

In this way:

  • if user wants another menu using Roots_Nav_Walker(), can simply copy and paste the code in header.php
  • user can add other custom Nav_Walkers
  • Widget menus can have as many level as user likes.

@JulienMelissas
Copy link
Contributor

👍 - I ran into this the other day and while I love what we do with the walker class, I don't like that it gets in the way sometimes.

@leopuleo If this is approved (this decision isn't up to me), we're going to ask you to squash your commits, so you may as well do that now 😉

@leopuleo
Copy link
Contributor Author

leopuleo commented Dec 1, 2014

Thanks @JulienMelissas, I prefere wait, so I don't have to squash twice if further commits are coming... 😄

@retlehs
Copy link
Member

retlehs commented Dec 1, 2014

👍 squash and we're good to go

@leopuleo
Copy link
Contributor Author

leopuleo commented Dec 1, 2014

Here we are..squashed! 😃

QWp6t added a commit that referenced this pull request Dec 2, 2014
Remove Roots_Nav_Walker as default for all menu
@QWp6t QWp6t merged commit 55c7976 into roots:master Dec 2, 2014
@leopuleo
Copy link
Contributor Author

leopuleo commented Dec 2, 2014

👍 Thank you guys!

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.

4 participants