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

Add @menu, @hasmenu and @endhasmenu #147

Merged
merged 3 commits into from
Jun 21, 2024
Merged

Add @menu, @hasmenu and @endhasmenu #147

merged 3 commits into from
Jun 21, 2024

Conversation

davidwebca
Copy link
Contributor

I love using your directives in blade files, but this one was missing for a while and I was initially debating about its utility, but I find myself not using Navi on smaller projects but hate using @php or passing ['echo' => false] in the arguments just to have those ugly exclamation brackets to echo it {!! !!}.

Conversations could be had about having @menu be smarter and do the "has_nav_menu" automatically which is the "normally expected behavior" (the nav location you call doesn't have anything = don't show anything), but in reality WordPress acquainted people could be thrown off by this since wp_nav_menu does some internal checks to always have something output in the navigation when the location has nothing in it like displaying the website's pages when no menu location has been set, thus allowing people to preview themes more easily. And so, I ended up deciding against it to keep this simple, but you be the final judge!

Tested locally on a real project, PHP 8.2, WordPress 6.5.

Thanks for looking at this.

@Log1x
Copy link
Owner

Log1x commented Jun 21, 2024

hey! im cool with this addition. we could maybe just add the has_nav_menu check onto @menu but also keep @hasmenu and @endhasmenu that way you can control container markup as well as use it standalone.

otherwise, just needs tests added to https://github.com/Log1x/sage-directives/blob/master/tests/Unit/WordPressTest.php and a mention in the docs.

edit: ahh re-reading what you said about the rendering – yeah, that makes sense. perhaps just leave the has_nav_menu stuff as-is then. :)

@davidwebca
Copy link
Contributor Author

Great, here you go, tests added!

@Log1x Log1x merged commit cb34494 into Log1x:master Jun 21, 2024
3 checks passed
@Log1x
Copy link
Owner

Log1x commented Jun 21, 2024

sweet!

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.

2 participants