Normalize the Navigation block appender behavior between canvas and list view contexts #71163
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
|
Size Change: +86 B (0%) Total Size: 1.92 MB
ℹ️ View Unchanged
|
getdave
commented
Aug 11, 2025
| onSelectOrClose( { | ||
| clientId: blockToInsert?.clientId, | ||
| } ); | ||
| onSelectOrClose( blockToInsert ); |
Contributor
Author
There was a problem hiding this comment.
This will be backwards compat as the new object will still contain clientId.
getdave
commented
Aug 11, 2025
| @@ -1,5 +1,6 @@ | |||
| export const DEFAULT_BLOCK = { | |||
| name: 'core/navigation-link', | |||
| attributes: { type: 'page', kind: 'post-type' }, | |||
Contributor
Author
There was a problem hiding this comment.
This is needed to ensure the directly inserted block is the correct variation (i.e. Page) for Nav blocks.
getdave
changed the title
getdave
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
getdave
commented
Aug 14, 2025
Comment on lines
25
to
32
| const directInsert = useSelect( | ||
| ( select ) => { | ||
| const { getBlockListSettings } = select( blockEditorStore ); | ||
| const settings = getBlockListSettings( clientId ); | ||
| return settings?.directInsert || false; | ||
| }, | ||
| [ clientId ] | ||
| ); |
Contributor
Author
There was a problem hiding this comment.
This will cause ALL list views that have an inserter to mirror the behaviour set in the block list settings. This makes sense to me and I believe is limited to the Nav block as it's the only one able to consume the PrivateListView.
Confidence check appreciated.
Contributor
There was a problem hiding this comment.
Aligning how it works makes sense to me. Not sure if we could move towards them using the same component/hook or if they're too fundamentally different.
Only code quality / performance feedback is that this useSelect could be merged with the one a few lines below to reduce the number of store subscriptions.
getdave
added
[Type] Enhancement
getdave
requested review from
andrewserong,
jeryj,
richtabor,
scruffian and
talldan
getdave
requested review from
ajitbohra,
ellatrix and
fabiankaegy
as code owners
|
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. |
Contributor
This works when I click in the sidebar to cancel link creation, but not when I click in the canvas: Screen.Recording.2025-08-14.at.13.42.33.mov |
Contributor
Author
|
This appears to be an issue relating to the canvas iframe. The Link UI is outside the iframe and thus the focusoutside logic doesn't seem to detect clicks within the editor canvas. If you click anywhere else in the editor UI other than the editor canvas the cleanup works fine. It's just selecting stuff inside the canvas. At this point I don't have a solution but essentially the |
getdave
force-pushed
the
feature/private-list-view-custom-appender
branch
from
92e661d to
b4c4291
Compare
getdave
commented
Aug 14, 2025
packages/block-library/src/navigation/edit/menu-inspector-controls.js
Outdated
Show resolved
Hide resolved
getdave
force-pushed
the
feature/private-list-view-custom-appender
branch
from
b4c4291 to
2c8a604
Compare
scruffian
reviewed
Aug 15, 2025
| isAppender | ||
| selectBlockOnInsert={ false } | ||
| shouldDirectInsert={ false } | ||
| shouldDirectInsert={ directInsert } |
Contributor
There was a problem hiding this comment.
I guess we don't have any instances where this is false anymore.
Contributor
Author
There was a problem hiding this comment.
It should mirror the block list settings.
getdave
commented
Aug 15, 2025
…in list view When using the 'Add block' feature in the Navigation block's list view context, the original auto-inserted navigation link block was not being cleaned up when a different block was selected via the QuickInserter. This caused duplicate blocks to remain in the list view. The fix adds a wrapper function handleSetInsertedBlock that removes the original inserted block before setting the new selected block, ensuring only the newly selected block remains in the list view. This aligns the list view behavior with the canvas context behavior.
…view When selecting a different block from the QuickInserter in the Navigation block's list view context, the original auto-inserted block was being removed with automatic block selection enabled. This caused focus stealing that closed the list view and switched to the canvas. The fix adds preventBlockSelection = false to removeBlock() calls to prevent automatic block selection, keeping the list view open and maintaining proper focus behavior.
…arity Rename the setInsertedBlock prop to onBlockInsert across the Navigation block components to make the prop name more generic and descriptive of its purpose as a callback function that's called when a block is inserted. This improves code readability and makes the prop name more reusable across different contexts while maintaining all existing functionality.
- Add useEditorCanvasCleanup hook to handle iframe interaction cleanup - Fixes issue where auto-inserted Navigation Link blocks weren't cleaned up when clicking on canvas blocks in List View context - Uses standardized iframe selector approach consistent with rest of codebase - Hook automatically manages event listener lifecycle based on LinkUI visibility
- Add useEditorCanvasCleanup hook to handle iframe interaction cleanup - Fixes issue where auto-inserted Navigation Link blocks weren't cleaned up when clicking on canvas blocks in List View context - Uses standardized iframe selector approach consistent with rest of codebase - Hook automatically manages event listener lifecycle based on LinkUI visibility
getdave
force-pushed
the
feature/private-list-view-custom-appender
branch
from
718929b to
b9cb6eb
Compare
Contributor
Author
|
I've tested this and it's looking pretty solid to me. Just waiting on a review. Update: turns out I forgot to push my changes 🤦 Should be working now @scruffian |
Contributor
Author
|
Thinking about follow ups here:
|
scruffian
approved these changes
Sep 3, 2025
Contributor
scruffian
left a comment
scruffian
left a comment
There was a problem hiding this comment.
This is working well now. A much better UX too, let's bring it in...
getdave
added a commit
that referenced
this pull request
Sep 4, 2025
- Create shared BackButton component to eliminate duplication - Replace duplicated back button implementations in LinkUIBlockInserter and LinkUIPageCreator - Standardize back button behavior and styling across components - Remove unused chevron icon imports from page-creator.js - Add proper JSDoc documentation for BackButton component This is the first phase of LinkUI duplication cleanup as discussed in PR #71163 follow-ups.
getdave
added a commit
that referenced
this pull request
Sep 4, 2025
- Create useDialogIds hook to eliminate duplicated useInstanceId calls - Replace duplicated accessibility ID generation in LinkUIBlockInserter and UnforwardedLinkUI - Maintain correct component parameters (LinkControl vs LinkUI) for proper ID generation - Standardize dialog accessibility ID pattern across components - Export hook for potential reuse in other components This is the second phase of LinkUI duplication cleanup as discussed in PR #71163 follow-ups.
getdave
added a commit
that referenced
this pull request
Sep 4, 2025
- Create shared DialogWrapper component that handles all dialog concerns internally - Eliminate useDialogIds hook by inlining useInstanceId calls directly in DialogWrapper - Derive component, componentName, and backButtonClassName from className prop - Move useFocusOnMount logic inside DialogWrapper for complete encapsulation - Replace duplicated dialog structures in LinkUIBlockInserter and LinkUIPageCreator - Fix useDialogIds reference error in main LinkUI component - Maintain all existing CSS styling and accessibility features - Fix nested ternary linting error with if/else statements This completes the major refactoring of LinkUI duplication cleanup as discussed in PR #71163 follow-ups.
This was referenced Sep 4, 2025
getdave
added a commit
that referenced
this pull request
Sep 5, 2025
* refactor: extract shared BackButton component from LinkUI - Create shared BackButton component to eliminate duplication - Replace duplicated back button implementations in LinkUIBlockInserter and LinkUIPageCreator - Standardize back button behavior and styling across components - Remove unused chevron icon imports from page-creator.js - Add proper JSDoc documentation for BackButton component Co-authored-by: getdave <[email protected]> Co-authored-by: scruffian <[email protected]> This is the first phase of LinkUI duplication cleanup as discussed in PR #71163 follow-ups. * refactor: extract useDialogIds hook for dialog accessibility IDs - Create useDialogIds hook to eliminate duplicated useInstanceId calls - Replace duplicated accessibility ID generation in LinkUIBlockInserter and UnforwardedLinkUI - Maintain correct component parameters (LinkControl vs LinkUI) for proper ID generation - Standardize dialog accessibility ID pattern across components - Export hook for potential reuse in other components This is the second phase of LinkUI duplication cleanup as discussed in PR #71163 follow-ups. * refactor: create DialogWrapper component with minimal API - Create shared DialogWrapper component that handles all dialog concerns internally - Eliminate useDialogIds hook by inlining useInstanceId calls directly in DialogWrapper - Derive component, componentName, and backButtonClassName from className prop - Move useFocusOnMount logic inside DialogWrapper for complete encapsulation - Replace duplicated dialog structures in LinkUIBlockInserter and LinkUIPageCreator - Fix useDialogIds reference error in main LinkUI component - Maintain all existing CSS styling and accessibility features - Fix nested ternary linting error with if/else statements This completes the major refactoring of LinkUI duplication cleanup as discussed in PR #71163 follow-ups. * refactor: extract LinkUIBlockInserter to separate file and consolidate BackButton - Extract LinkUIBlockInserter component to block-inserter.js for consistency with page-creator.js - Consolidate BackButton component into dialog-wrapper.js since it's only used there - Fix import issues with privateApis in block-inserter.js - Remove separate back-button.js file to reduce file count - Clean up unused imports in link-ui.js after component extraction - Fix JSDoc alignment in block-inserter.js - Maintain all existing functionality while improving file organization This completes the component extraction phase of the LinkUI duplication cleanup. * refactor: simplify DialogWrapper instance ID generation Use DialogWrapper as the component for useInstanceId instead of deriving it from className. This removes unnecessary complexity and unused imports. * Standardise ID generation in Dialog
This was referenced Nov 14, 2025
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
This PR aligns the behavior of Navigation block appenders between canvas and list view contexts. Currently, the list view appender uses a different insertion flow than the canvas appender, leading to inconsistent user experience.
Why
The Navigation block's list view and canvas contexts should provide consistent appender behavior. Users expect the same insertion experience regardless of whether they're editing in the canvas or using the list view sidebar. This alignment improves the overall user experience and reduces confusion.
This should open the door for improved UX in #71192.
How
directInsertanddefaultBlocksettings configured in the Navigation blockKnown follow ups
+inserter tooltip to beAdd pagein both List View and canvas.Testing
Screenshots
Screen.Capture.on.2025-08-14.at.11-46-06.mp4
Key Changes from Trunk
directInsertanddefaultBlocksettings