Simplify and improve navigation link creation flow #73210
Merged
+340
−59
Conversation
This file contains hidden or 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
|
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. |
|
Size Change: +165 B (+0.01%) Total Size: 2.51 MB
ℹ️ View Unchanged
|
jeryj
added
[Block] Navigation Link
jeryj
commented
Nov 12, 2025
| // be lost during the new block selection process. | ||
| useEffect( () => { | ||
| if ( isNewLink.current && isSelected && ! url ) { | ||
| if ( isNewLink.current && isSelected ) { |
Contributor
Author
There was a problem hiding this comment.
! url check is unnecessary since ! url is already part of the isNewLink check.
|
|
||
| if ( | ||
| ! url || | ||
| ( ! url && ! ( hasUrlBinding && isBoundEntityAvailable ) ) || |
Contributor
Author
There was a problem hiding this comment.
The ! ( hasUrlBinding && isBoundEntityAvailable ) check is necessary as bound links will evaluate as null while resolving.
Contributor
There was a problem hiding this comment.
Can we combine this and the same code on line 493 into one const?
Comment on lines
444
to
496
| ( ! url && ! ( hasUrlBinding && isBoundEntityAvailable ) ) || | ||
| isInvalid || | ||
| isDraft || | ||
| ( hasUrlBinding && ! isBoundEntityAvailable ), |
Contributor
Author
There was a problem hiding this comment.
I feel like ( ! url && ! ( hasUrlBinding && isBoundEntityAvailable ) ) and ( hasUrlBinding && ! isBoundEntityAvailable ) should be within the isInvalid check instead of being separate conditions. Maybe this should get done AFTER the RC though?
getdave
reviewed
Nov 13, 2025
Contributor
getdave
left a comment
getdave
left a comment
There was a problem hiding this comment.
This is very close. I found one edge case which appears to be when moving from a custom link to an synced link. See screenshot.
The popover immediately opens on the new link.
Screen.Capture.on.2025-11-13.at.15-34-21.mp4
Contributor
Author
|
@getdave I can move from custom url + link popover open to a synced page and it works fine. I think there's something wrong with entity binding and tags on trunk. This screen recording is from trunk. I added a tag block to the navigation menu and it shows the purple sync icon but the sync error message appears: Screen.Recording.2025-11-13.at.11.31.49.AM.mov |
Contributor
Author
|
video.webm 
If change the text from "Uncategorized" to "Cat" in the test, it passes consistently. This is the difference between a synced page (Cat) and a category (Uncategorized) It's possible this is just due to the Add Block flow and not specific to pages though |
jeryj
commented
Nov 13, 2025
jeryj
force-pushed
the
fix/popover-reselects-nav-link
branch
from
c40506c to
1a3042a
Compare
Contributor
|
Are we still targetting WP 6.9 for this? |
jeryj
changed the title
Contributor
Author
I think it's too big of a change. I've simplified the case to fix the most serious bug in #73368, but will save the larger ux and code change for 7.0 |
Member
|
Let’s please change the labels accordingly 🙂 |
scruffian
approved these changes
Nov 18, 2025
Contributor
scruffian
left a comment
scruffian
left a comment
There was a problem hiding this comment.
Looks good and works as described. Just left a few nits...
| const isNewLink = useRef( ! url ); | ||
| // A link is "new" only if it has an undefined label | ||
| // After the link is created, even if no label is provided, it's set to an empty string. | ||
| const isNewLink = useRef( label === undefined ); |
Contributor
There was a problem hiding this comment.
In #73368 we use a different method to check isNewLink.
const isNewLink = useRef( ! url && ! metadata?.bindings?.url );
Should we use that here?
Contributor
Author
There was a problem hiding this comment.
label is actually more reliable. It's always undefined when a new link is created, then after the link is created it becomes an empty string.
| hasChildren: !! getBlockCount( clientId ), | ||
| validateLinkStatus: enableLinkStatusValidation, | ||
| parentBlockClientId: parentBlockId, | ||
| isSubmenu: parentBlockName === 'core/navigation-submenu', |
Contributor
There was a problem hiding this comment.
Just thinking... If we combine these two blocks into one we're going to need a different way to identify submenus...
Contributor
There was a problem hiding this comment.
Likely an attribute. isSubmenu.
|
|
||
| if ( | ||
| ! url || | ||
| ( ! url && ! ( hasUrlBinding && isBoundEntityAvailable ) ) || |
Contributor
There was a problem hiding this comment.
Can we combine this and the same code on line 493 into one const?
jeryj
force-pushed
the
fix/popover-reselects-nav-link
branch
from
822ac6a to
d71040c
Compare
jeryj
removed
the
Backport to WP 6.9 Beta/RC
jeryj
commented
Nov 19, 2025
| * Cancels editing state. | ||
| */ | ||
| const stopEditing = () => { | ||
| isEndingEditWithFocusRef.current = !! wrapperNode.current?.contains( |
Contributor
Author
There was a problem hiding this comment.
isEndingEditWithFocusRef.current was being set and no longer used. Removing it for cleanup.
scruffian
reviewed
Nov 19, 2025
| const textInputRef = useRef(); | ||
| const searchInputRef = useRef(); | ||
| const isEndingEditWithFocusRef = useRef( false ); | ||
| // TODO: Remove entityUrlFallbackRef and previewValue in favor of value prop after taxonomy entity binding |
Contributor
There was a problem hiding this comment.
Thanks for documenting this here, it will make it a lot easier to remove it when the time comes.
…ing block selection
jeryj
force-pushed
the
fix/popover-reselects-nav-link
branch
from
bad68e1 to
a7aea0d
Compare
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 #73171 and #73178
In #72142, block selection was changed to happen at the END of the cycle (onClose). This helped remove some unnecessary states, but caused an issue where your block inspector controls are not available for your new nav block until you close the link ui. This PR updates block selection to happen at the earliest moment we have a stable block - the onChange event from the LinkUI.
There is one workaround in which we need to use a query selector to access the submenu link appender when a new submenu link is created. This is the only case a new link is added without using the block appender. The first submenu link item is added when pressing the submenu button on the top level navigation link item toolbar. All other instances we can let focus naturally return to the ad link appender.
Why?
Simplify the code, and shows the newly added link in the inspector sidebar as soon as possible. Allows an easier and faster navigation menu creation process.
How?
Within the onChange event from the LinkUI when adding a new link, call
selectBlockon the newly added link with the second parameter as nulll so it does not send focus to the block. This allows the link to stay focused and open, while having the sidebar inspector show the newly selected navigation link item information. This is a more similar behavior to how it was before 6.9.Testing Instructions
adding a navigation link
adding a new submenu
adding a submenu navigation link
Testing Instructions for Keyboard
Screenshots or screencast
Before
After