Fix create menu after menu switch #59630
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +1.08 kB (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
draganescu
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
scruffian
previously approved these changes
Mar 6, 2024
|
@scruffian @afercia @draganescu As this bug was uncovered during the RC phase do you feel that it should be included in the WP 6.5? If so why? Noting that if required, we will need to present a rationale to the release team to clear this to land. As a member of that team I would prefer not to cast my opinion without first receiving your feedback. Much appreciated. |
|
100% it should be included in PW 6.5 because it's a severe bug (unable to access the create functionality) of a block central to developing a website which is what WP does. It also is a bug that I think was there for at least one release. |
… able to clean state after ref is updated
draganescu
force-pushed
the
fix/create-new-menu
branch
from
6a3df80 to
c2c53d1
Compare
This comment was marked as outdated.
This comment was marked as outdated.
draganescu
force-pushed
the
fix/create-new-menu
branch
from
c6b9ef3 to
1e45317
Compare
jeryj
reviewed
Mar 7, 2024
packages/block-library/src/navigation/edit/navigation-menu-selector.js
Outdated
Show resolved
Hide resolved
getdave
reviewed
Mar 8, 2024
…sed on resolving menus
|
I have updated the I've updated the tests accordingly to take into account that the disabled state depends on The more I spend time here the more useless |
getdave
reviewed
Mar 11, 2024
There was a problem hiding this comment.
Thanks for this PR and working on a fix to this bug.
I see the original bug is resolved with the Create menu option enabled even when you switch menus.
However, if I click Create new menu I would expect the options to switch my menu to be disabled during that process's resolution. Currently they are not for both block and classic menus you might have.
getdave
reviewed
Mar 11, 2024
packages/block-library/src/navigation/edit/test/navigation-menu-selector.js
Show resolved
Hide resolved
… undo the focus update
getdave
reviewed
Mar 11, 2024
getdave
approved these changes
Mar 11, 2024
getdave
reviewed
Mar 11, 2024
Comment on lines
130
to
137
| hasResolvedNavigationMenus, | ||
| createNavigationMenuIsSuccess, | ||
| canUserCreateNavigationMenu, | ||
| createNavigationMenuIsError, | ||
| isCreatingMenu, | ||
| isUpdatingMenuRef, | ||
| menuUnavailable, | ||
| noBlockMenus, | ||
| noMenuSelected, |
There was a problem hiding this comment.
We should be able to remove all the unused deps from this effect.
I propose we do this in a followup.
getdave
pushed a commit
that referenced
this pull request
Mar 11, 2024
* don't set creating menu flag on menu switch * return Promises from the navigation menu ref changing functions to be able to clean state after ref is updated * update tests * await the menu creation * stub test updates * revert test changes and remove decorative promises * do not disable the UI when changing the navigation, disable the UI based on resolving menus * block the UI for menu switching too, update tests with some comments, undo the focus update * properly disable menu choices via their choices prop Co-authored-by: draganescu <[email protected]> Co-authored-by: scruffian <[email protected]> Co-authored-by: jeryj <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: afercia <[email protected]> # Conflicts: # packages/block-library/src/navigation/edit/test/navigation-menu-selector.js
|
I just manually cherry-picked this PR to the pick/wp-65-rc-2 branch to get it included in the next release |
getdave
added
Backport to WP 6.7 Beta/RC
getdave
pushed a commit
that referenced
this pull request
Mar 12, 2024
* don't set creating menu flag on menu switch * return Promises from the navigation menu ref changing functions to be able to clean state after ref is updated * update tests * await the menu creation * stub test updates * revert test changes and remove decorative promises * do not disable the UI when changing the navigation, disable the UI based on resolving menus * block the UI for menu switching too, update tests with some comments, undo the focus update * properly disable menu choices via their choices prop Co-authored-by: draganescu <[email protected]> Co-authored-by: scruffian <[email protected]> Co-authored-by: jeryj <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: afercia <[email protected]> # Conflicts: # packages/block-library/src/navigation/edit/test/navigation-menu-selector.js
|
While this PR fixes the disabled state of the 'Create new menu' button, it introduces a regression. There is now a focus loss when creating a new menu because the whole dropdown menu stays open but all menu items gets 'disabled', including the menu item that had keyboard focus. It is important to point out, once again, a very important principle: Important Never, ever, disable controls dynamically while they have focus. I'd appreciate that this kind of changes are tested with keyboard as clearly this wasn't the case in this PR. Thanks. I will create a separate issue, and this needs to be addressed soon as it's a regression in RC. |
Agreed. Are you able to tackle this one? The next RC is RC3 on 19th March. |
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Mar 27, 2024
* don't set creating menu flag on menu switch * return Promises from the navigation menu ref changing functions to be able to clean state after ref is updated * update tests * await the menu creation * stub test updates * revert test changes and remove decorative promises * do not disable the UI when changing the navigation, disable the UI based on resolving menus * block the UI for menu switching too, update tests with some comments, undo the focus update * properly disable menu choices via their choices prop Co-authored-by: draganescu <[email protected]> Co-authored-by: scruffian <[email protected]> Co-authored-by: jeryj <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: afercia <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Closes #59604 - create menu is available as an option after menu switching.
Why?
Bug fix.
How?
Removes a seemingly useless? call to setting a state about creating a menu (
setIsCreatingMenu) which makes no sense in the context of switching a menu.Testing Instructions
Testing Instructions for Keyboard
N/A
Screenshots or screencast
create-new-menu.mp4