Display inserter popover in offcanvas UI #46013
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.32 MB
ℹ️ View Unchanged
|
getdave
force-pushed
the
try/display-inserter-popover-in-offcanvas
branch
from
4fa1197 to
550e338
Compare
getdave
commented
Nov 25, 2022
|
|
||
| if ( onSelectOrClose ) { | ||
| onSelectOrClose(); | ||
| onSelectOrClose( { | ||
| insertedBlock: blockToInsert, |
There was a problem hiding this comment.
As the block isn't "selected" this is the only reference we have to the block that was just inserted.
getdave
commented
Nov 25, 2022
Comment on lines
51
to
68
| value={ link } | ||
| linkAttributes={ { | ||
| type: insertedBlock.attributes.type, | ||
| url: insertedBlock.attributes.url, | ||
| kind: insertedBlock.attributes.kind, | ||
| } } |
There was a problem hiding this comment.
We should probably make this one prop here and then split it out into value to pass down into <LinkControl>.
getdave
commented
Nov 25, 2022
| @@ -0,0 +1,103 @@ | |||
| /** | |||
There was a problem hiding this comment.
Ignore this. It's copy/pasted.
There was a problem hiding this comment.
Should we add a note to the code about that?
getdave
commented
Nov 25, 2022
getdave
commented
Nov 28, 2022
| blockToInsert, | ||
| getInsertionIndex(), | ||
| rootClientId, | ||
| selectBlockOnInsert |
There was a problem hiding this comment.
If undefined this will default to true meaning that the blocks will continue to be focused in other contexts.
getdave
commented
Nov 28, 2022
|
|
||
| if ( onSelectOrClose ) { | ||
| onSelectOrClose(); | ||
| onSelectOrClose( { | ||
| insertedBlockId: blockToInsert?.clientId, |
There was a problem hiding this comment.
This is what provides us with access to information about the block that was just inserted.
getdave
commented
Nov 28, 2022
Comment on lines
16
to
17
| if ( value?.title && value.title !== internalInputValue ) { | ||
| setInternalInputValue( value.title ); | ||
| if ( value && value !== internalInputValue ) { | ||
| setInternalInputValue( value ); |
There was a problem hiding this comment.
Ignore - will be rebased out by #46113 (reviews on that appreciated 🙏 )
getdave
commented
Nov 28, 2022
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { store as blockEditorStore } from '../../store'; | ||
| import Inserter from '../inserter'; | ||
| import { LinkUI } from './link-ui'; | ||
| import { updateAttributes } from './update-attributes'; |
There was a problem hiding this comment.
This holds a lot of complex logic so we must borrow it from the implementation in Navigation Link block
getdave
commented
Nov 28, 2022
Comment on lines
+34
to
+43
| const { insertedBlockAttributes } = useSelect( | ||
| ( select ) => { | ||
| const { getBlockAttributes } = select( blockEditorStore ); | ||
|
|
||
| return { | ||
| insertedBlockAttributes: getBlockAttributes( insertedBlock ), | ||
| }; | ||
| }, | ||
| [ insertedBlock ] | ||
| ); |
There was a problem hiding this comment.
When the inserted block ID changes we need to refetch info about the block attributes so we can update the Link UI.
getdave
commented
Nov 28, 2022
Comment on lines
+47
to
+50
| const setAttributes = | ||
| ( insertedBlockClientId ) => ( _updatedAttributes ) => { | ||
| updateBlockAttributes( insertedBlockClientId, _updatedAttributes ); | ||
| }; |
There was a problem hiding this comment.
We provide a customised implementation of setAttributes to the updateAttributes method. This is because in this context we do not have access to the block's setAttributes method. Therefore we must dispatch attribute updates directly to the store.
The function is curried in order that we can partially apply the blockID in advance.
getdave
requested review from
scruffian,
MaggieCabrera,
ajlende and
draganescu
getdave
force-pushed
the
try/display-inserter-popover-in-offcanvas
branch
from
670a3fc to
1692dfe
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This will make this easier to refactor in future
Co-authored-by: Dave Smith <getdavemail@gmail.com>
scruffian
force-pushed
the
try/display-inserter-popover-in-offcanvas
branch
from
168c62b to
383faa3
Compare
|
This is looking good. The only change I would suggest is to close the Link UI once a link is selected. At the moment it's quite hard to add another block once you've added one... link.appender.mp4 |
|
I added a commit to hide the Link UI once a link is selected. |
|
@scruffian What's left before we can look to merge this one? |
If you're happy with my change, then I think we can merge :) |
getdave
commented
Nov 30, 2022
Comment on lines
+76
to
+80
| const featuredBlocks = [ | ||
| 'core/site-logo', | ||
| 'core/social-links', | ||
| 'core/search', | ||
| ]; |
There was a problem hiding this comment.
I will track this in #46166
4 tasks
getdave
commented
Nov 30, 2022
| * @typedef {'post-type'|'custom'|'taxonomy'|'post-type-archive'} WPNavigationLinkKind | ||
| */ | ||
| /** | ||
| * Navigation Link Block Attributes |
There was a problem hiding this comment.
Also needs tracking in #46166
|
Follow ups:
|
This was referenced Nov 30, 2022
mpkelly
pushed a commit
to mpkelly/gutenberg
that referenced
this pull request
Dec 7, 2022
) * Add new prop to control 4th arg to insertBlock * Don’t select block on insertion from offcanvas appender * Make appender * Provide data on inserted block in Inserter block callback * Extract updater function from Nav Link block This will make this easier to refactor in future * Extract standalone Link UI component from Nav Link block * Rename Link UI props * Remove need for replaceBlock prop and colocate with component * Rename more props * Remove deps on attributes in favour of generic updater prop * Colocate transforms control with Link UI component * Copy Link UI from Nav Link Block * Experiment with including Link UI in offcanvas * Select inserted link attributes to dynamically update UI * Pass title and new tab props to Link UI * Remove create suggestion and resulting dep on Core Data package * Use WP version of escape html over lodash dep * Update packages/block-editor/src/components/off-canvas-editor/link-ui.js Co-authored-by: Dave Smith <getdavemail@gmail.com> * Separately import to fix unit tests * Fix lint * Use the correct import! * Close the link appender Co-authored-by: Ben Dwyer <ben@scruffian.com>
82 tasks
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?
Shows the block inserter popover within the offcanvas UI area. Currently on
trunkthis displays next to the block in question.🙏 🙏 Please read the
Howsection below before reviewing this PR. 🙏🙏Closes #45996
Why?
The user has clicked the appender in the offcanvas and thus the interaction of choosing the block should remain inline within the offcanvas.
How?
To make this PR work there is a precondition of the UX that we need to be aware of - namely that when a block is inserted using the appender we need to disable the selection of the block being inserted.
Why? Because if we allow the block to be selected then the sidebar (inspector controls) will update to display the settings of that [inserted] block rather than remaining on the Navigation block. This would immediately hide the offcanvas menu and pull the user out of the context of creating their Menu which we believe would not be a good flow.
This PR adds a new prop to the global
<Inserter>to allow configuring whether or not to select the block upon insertion.Caveats
core/navigation-linkblock is inserted. We should tackle that in a follow up.Testing Instructions
Screenshots or screencast
Screen.Capture.on.2022-11-28.at.10-58-55.mp4