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

Migrate classic menus to block-based menus on theme switch #36255

Merged
merged 21 commits into from
Nov 5, 2021

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Nov 5, 2021

Description

Adds an action that acts on theme_switch and:

  • Short-circuits if the new theme isn't a FSE theme
  • Finds all the menus assigned to a location in the old theme
  • Migrates each to a wp_navigation post supported by the navigation block
  • Updates fse_navigation_areas to reflect these migrated menus

gutenberg_migrate_nav_on_theme_switch is the only new function, the other ones were copied over from the navigation block. It would be great to remove the duplication in a follow-up PR.

How has this been tested?

  • Switch to any classic theme
  • Setup a few menus in a few locations, the locations should be called either primary, secondary, or tertiary
  • Switch to tt1-blocks
  • Confirm the menus were migrated as expected

lib/navigation.php Outdated Show resolved Hide resolved
lib/navigation.php Outdated Show resolved Hide resolved
lib/navigation.php Outdated Show resolved Hide resolved
lib/navigation.php Outdated Show resolved Hide resolved
lib/navigation.php Outdated Show resolved Hide resolved
@adamziel adamziel requested a review from talldan November 5, 2021 15:06
@adamziel adamziel self-assigned this Nov 5, 2021
@adamziel adamziel added the [Block] Navigation Affects the Navigation Block label Nov 5, 2021
@anton-vlasenko anton-vlasenko self-requested a review November 5, 2021 15:13
Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

LGTM! 🚢

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

Works well. I noticed one thing. When you add a new Nav block you get duplicate choices under the Select existing menu dropdown in the placeholder state:

Screen Shot 2021-11-05 at 15 31 29

@adamziel
Copy link
Contributor Author

adamziel commented Nov 5, 2021

Good find @getdave, let's address this in a follow-up PR

@draganescu draganescu merged commit d0c695c into trunk Nov 5, 2021
@draganescu draganescu deleted the try/migrate-classic-menus-on-theme-switch branch November 5, 2021 15:55
@draganescu
Copy link
Contributor

Merged with "admin" because all checks were green except build artefact with was stuck on some network problem.

@github-actions github-actions bot added this to the Gutenberg 11.9 milestone Nov 5, 2021
@adamziel adamziel mentioned this pull request Nov 5, 2021
@spacedmonkey
Copy link
Member

What about menus that used in widgets / hardcoded in templates. Locations are not the only way to know if a menu is being used.

@getdave
Copy link
Contributor

getdave commented Nov 8, 2021

Good find @getdave, let's address this in a follow-up PR

Did we create a follow up or should I look into that? I created a follow up in #36307

@getdave
Copy link
Contributor

getdave commented Nov 8, 2021

What about menus that used in widgets / hardcoded in templates. Locations are not the only way to know if a menu is being used.

Nonetheless, I'd say the locations route covers the 80% use case. Widgets/hard coding is possible, and completely valid, but seems less likely. Do you have any ideas for route we could all explore that could help cover those scenarios [the ones you mentioned] as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants