Nav offcanvas - handle non-direct insert block inserter #46503
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
getdave
changed the title
|
Size Change: +289 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
getdave
added
[Block] Navigation
getdave
commented
Dec 13, 2022
Comment on lines
+154
to
+157
| const firstBlock = | ||
| Array.isArray( blocks ) && blocks?.length | ||
| ? blocks[ 0 ] | ||
| : blocks; |
There was a problem hiding this comment.
I'm not sure whether this is necessary. Can anyone think of a scenario when onSelect would be called by the quick inserter with an array of blocks rather than just a single block?
There was a problem hiding this comment.
Now that #46531 has landed I may refactor this callback implementation in a follow up.
There was a problem hiding this comment.
The contact form in Jetpack has blocks inside it for each field. I'm not sure if they get appended as an array though.
getdave
commented
Dec 13, 2022
| }; | ||
|
|
||
| const maybeSetInsertedBlockOnInsertion = ( _insertedBlock ) => { | ||
| if ( ! _insertedBlock?.clientId ) { |
There was a problem hiding this comment.
The Inserter can be closed with no block selected. In this case the callback is still closed but no block is passed to the callback. This is the guard.
getdave
commented
Dec 13, 2022
| setInsertedBlock( insertedBlockId ); | ||
| } } | ||
| onSelectOrClose={ maybeSetInsertedBlockOnInsertion } | ||
| __experimentalIsQuick |
There was a problem hiding this comment.
This triggers the smaller inserter.
getdave
requested review from
scruffian,
MaggieCabrera,
georgeh,
ajlende,
artemiomorales and
draganescu
getdave
commented
Dec 13, 2022
| @@ -39,6 +39,7 @@ function useInsertionPoint( { | |||
| isAppender, | |||
| onSelect, | |||
| shouldFocusBlock = true, | |||
| selectBlockOnInsert = true, | |||
There was a problem hiding this comment.
Here I'm aiming to ensure backwards compat by setting the default to the same as the hardcoded original value.
There was a problem hiding this comment.
Should we make a comment for the future?
There was a problem hiding this comment.
I don't know. We could but I'm thinking someone coming here and flipping that value would think twice before changing the component default.s
MaggieCabrera
approved these changes
Dec 14, 2022
|
Unlike in the video, when I insert a custom link after having inserted another block type, the inspector opens on the canvas, not in the sidebar. |
dmsnell
pushed a commit
that referenced
this pull request
Dec 15, 2022
* Pass blocks from block inserter to custom appender callback * Allow only blocks which support Link UI to trigger it’s rendering on insertion * Allow Link UI for all supporting blocks * Extract clearly named function to improve comprehension * Allow for programmatic disabling of selecting block on insertion via Quick Inserter * Fix stray false * Provide correct default to ensure behaviour is backwards compatible * Always pass selectBlockOnInsert
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?
Handles the offcanvas UX block insertion flow in the Nav offcanvas experiment when the Nav block contains blocks other than
core/navigation-linkthus triggering the "full" inserter UI (i.e. no direct-insert). The flow becomes:Browsein the quick inserter triggers the full block inserter and closes the off canvas 🤔Closes #46203
Why?
The two-step insertion behaviour is what already happens in the editor canvas when the Nav block has non-
core/navigation-linkblocks in it. Therefore we need to handle the same use case in the offcanvas.Having a two step process affords the user greater control over the type of block they wish to insert into their Navigation.
Whilst this does introduce some friction for those users wishing simply to add links, it does afford full power to insert any of the block types without having to rely on obfuscated/convoluted UX such as:
How?
The key changes are:
__experimentalIsQuickto the<Inserter>component to trigger the<QuickInserter>style (as per block canvas)selectBlockOnInsertand conditionally set the 4th argument of calls toinsertBlockwithin the hooks consumed by the<QuickInserter>onSelectOrCloseand have the<QuickInserter>call this when defined with the blocks that were inserted (if any).Future plans
In the future I imagine we can refactor some of this callback code around the Link UI:
add a reducer to keep track of the inserted blocks as a flattened arraysee Add new selector getLastInsertedBlockClientId #46531add a selector to retrieve the most recently inserted block (e.g.see Add new selector getLastInsertedBlockClientId #46531getLastInsertedBlock())ListViewBlockContentscomponentgetLastInsertedBlockselectorTesting Instructions
core/navigation-linkthis should work as pertrunkand immediately insert the block and show the Link UI. That is expected.Transformsin the Link UI to add a social icons block.Testing Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2022-12-13.at.13-31-35.mp4