Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
Nov 5, 2021
Merged

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

merged 21 commits into from
Nov 5, 2021

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Nov 5, 2021 •
edited
Loading

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

draganescu
draganescu reviewed Nov 5, 2021
View reviewed changes
adamziel
adamziel commented Nov 5, 2021
View reviewed changes
adamziel
adamziel commented Nov 5, 2021
View reviewed changes
draganescu
draganescu reviewed Nov 5, 2021
View reviewed changes
draganescu
draganescu reviewed Nov 5, 2021
View reviewed changes
@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
draganescu
draganescu approved these changes Nov 5, 2021
View reviewed changes
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! 🚢

getdave
getdave approved these changes Nov 5, 2021
View reviewed changes
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
Merged
@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 •
edited
Loading

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?

@getdave getdave mentioned this pull request Nov 8, 2021
Closed
@talldan talldan mentioned this pull request Nov 9, 2021
Merged
@getdave getdave mentioned this pull request Nov 9, 2021
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@draganescu draganescu draganescu approved these changes

@getdave getdave getdave approved these changes

@spacedmonkey spacedmonkey Awaiting requested review from spacedmonkey spacedmonkey is a code owner

@tellthemachines tellthemachines Awaiting requested review from tellthemachines

@TimothyBJacobs TimothyBJacobs Awaiting requested review from TimothyBJacobs

@talldan talldan Awaiting requested review from talldan

@anton-vlasenko anton-vlasenko Awaiting requested review from anton-vlasenko

Assignees

@adamziel adamziel

Labels
[Block] Navigation Affects the Navigation Block
Projects
None yet
Milestone
Gutenberg 11.9
Development

Successfully merging this pull request may close these issues.
4 participants
@adamziel @draganescu @spacedmonkey @getdave