Skip to content
This repository has been archived by the owner on Jan 10, 2018. It is now read-only.

Remove wp_nav_menu fallback #458

Merged
merged 9 commits into from
Oct 18, 2016
Merged

Remove wp_nav_menu fallback #458

merged 9 commits into from
Oct 18, 2016

Conversation

enodekciw
Copy link
Contributor

#425

  • Sets wp_nav_menu fallback_cb to false
  • Removes twentyseventeen_fallback_menu(), which isn't needed anymore
  • Checks whether the 'top' theme_location has a menu assigned or not. If not - prevents any navigation menu markup from showing up.

@samikeijonen
Copy link
Contributor

Couple of notes:

  1. When using has_nav_menu there is no need for setting fallback at all. We don't get that far.
  2. Now there is also no need for adding dropdown icon via JS. There is also couple of lines CSS which can be removed.

On my mobile so harder to give more details and line numbers.

@enodekciw
Copy link
Contributor Author

enodekciw commented Oct 18, 2016

@samikeijonen

  1. Default wp_nav_menu() fallback_cb is set to 'wp_page_menu'. So we do need to set it to false, so that wp_nav_menu doesn't fallback to 'wp_page_menu'. See codex: Set to false for no callback.
  2. Good point. I'll dig into this right now, since I realised that now we dont need to enqueue /assets/js/navigation.js if no menu is assigned.

@samikeijonen
Copy link
Contributor

If you use has_nav_menu around nav markup there is no nav at all unless you set one. Therefore wp_nav_menu won't run. But it doesn't hurt to have fallback either.

@enodekciw
Copy link
Contributor Author

Oh, now it makes sense. Guess my brain is still sleeping. Sorry :)

@melchoyce
Copy link
Contributor

This is looking really good!

I'm wondering if there’s any way we could make the scrollability more obvious, maybe by adding a down arrow over the header image so there’s some indication that you can scroll more. Or not taking up the whole viewport, so a little bit of the page background peeks through at the bottom.

I'm also totally cool with merging this and then exploring enhancements like that in a new PR. :)

@laurelfulford
Copy link
Collaborator

@melchoyce, moved your suggestion to #472 🙂

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants