-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Revert: Navigation: Always create a fallback menu #48602
Conversation
Size Change: -31 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
Flaky tests detected in 38fc111. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4354738301
|
If we go with #48625 then we won't need this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should revert this because while it solves numerous problems it introduces others which seem worse. This is still a valuable approach.
Maybe the work to do it serverside on a wp action will be a better approach. Maybe the work to just have an utility endpoint that handles all navigation fallback cases will be a better approach.
But having the pressure of fixing what always creating from client breaks will make for rushed choices.
When cherry-picking this PR to the 15.3 branch and resolving the conflicts with changes made in #48680, I'm getting several linting errors that prevent me from committing the cherry-picked changes to the branch. I'm wondering if cherry-picking #48680, too, would be better than trying to fix the linting errors myself. In any case, I don't think this is a blocking issue for 15.3, so I'm going to proceed with the release but will mark this PR as a backport for a 15.3 minor release. I'm happy to pair with somebody to speed up resolving the issues. |
I think this probably is a blocker for 15.3 as without this change we're going to see a lot of duplicate menus on user's sites. #48680 introduced other bugs, so if you include that you'll also need to include the bug fixes that followed from it. Are the linting errors about exhaustive deps? If so I suggest we just ignore them for this release, as this is a new linting rule and we're still updating things to comply with it. |
Once I solved the conflicts and manually adjusted errors (like duplicate declarations of |
I can't, since I'm getting TypeScript errors. @draganescu reached out to me so I'll work with him on this issue, thanks! |
This reverts Navigation: Always create a fallback menu
This PR was assuming that there is only one navigation block on a page. With multiple navigation blocks, we end up creating multiple wp_navigation menus. We should solve that issue before we proceed with that PR.