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

Blockbase: A different approach to navigation #4575

Closed
wants to merge 6 commits into from

Conversation

scruffian
Copy link
Member

@scruffian scruffian commented Sep 10, 2021

Changes proposed in this Pull Request:

In order to add social links to a navigation block (as suggested in #4482) using the Customizer, I believe we need a different approach to navigation. One problem with the current approach is that if a user edits their navigation in the Site Editor, edits can no longer be made in Customizer. Another issue is that our approach to the social menu needs significant rework by the user when they switch to the Site Editor.

This PR proposes a different approach:

  • We create a new template part called "navigation"
  • When a user edits their menus in the Customizer, we recreate the same menu in the template part created above

The major benefit of this approach is that we can build a navigation block for the Site Editor in any way we want. We can use the nav walker class to convert a PHP nav menu to a block nav menu. It's surprisingly simple!

The only downside with this approach is that if I edit the template via the Site Editor, or the Template editor, and edit it again in the customizer, or menus screen, my previous changes would be overwritten.

To test:

  • Switch to Blockbase (or any blockbase family theme)
  • Create a menu at the "social" location
  • Confirm that social links are added to the navigation

Still to do:

  • Get the social links to display on the same line as the primary navigation
  • Get the preview in customizer to update in real time (this is an issue with the current approach)

@scruffian scruffian requested a review from a team September 10, 2021 21:10
@scruffian scruffian self-assigned this Sep 10, 2021
@scruffian scruffian marked this pull request as ready for review September 14, 2021 22:40
@mikachan
Copy link
Member

I've tested this with Blockbase by adding a menu to the 'social' location. Everything seems to work great - it works at all resolutions and I'm able to edit it via the Menus section and the Customizer. Here's what I'm seeing:

Desktop:
Screenshot 2021-09-15 at 11 46 25

Small resolutions:
Screenshot 2021-09-15 at 11 47 34

Sorry I haven't got any more meaningful feedback... I also didn't see any errors or warnings while testing.

@scruffian scruffian requested a review from kjellr September 15, 2021 12:51
@kjellr
Copy link
Contributor

kjellr commented Sep 16, 2021

I'm getting some errors when trying to run this PR on a fresh site:

Screen Shot 2021-09-16 at 7 56 37 AM

@scruffian
Copy link
Member Author

scruffian commented Sep 17, 2021

@kjellr I can't replicate that, but I've added a commit which I hope will address it. How did you get to an "fresh site" state?

@kjellr
Copy link
Contributor

kjellr commented Sep 17, 2021

All sorted out now. This works great! The only small thing I noticed is that if you enter an email address/mailto link, it creates just a standard custom link block, not a mail one.

@draganescu
Copy link

Howdy! This is cool. Nevertheless it feels like a serious workaround, a very specific implementation of this theme's need. I wonder how could the navigation block change to allow one to implement "universal theme blocks-in-menus" easier?

There is ongoing effort to change how the navigation block stores its data. It would be lovely if we could chime in with the theme challenges, described in detail, so that the foundational design of the block (which is still evolving) takes them into account as early as possible.

@scruffian
Copy link
Member Author

Closing in favour of #4482

@scruffian scruffian closed this Oct 1, 2021
@scruffian scruffian deleted the update/blockbase-navigation branch October 1, 2021 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants