Simplify insertion point #30301
Merged
Simplify insertion point #30301
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
ellatrix
commented
Mar 26, 2021
| Sets the insertion point and shows it to users. | ||
|
|
||
| Components like <Inserter> will default to inserting blocks at this point. | ||
| Returns an action object used in signalling that the insertion point should |
Member
Author
There was a problem hiding this comment.
Please note that I did a partial revert of 7f1fac9.
ellatrix
commented
Mar 26, 2021
| case 'INSERT_BLOCKS': | ||
| case 'REMOVE_BLOCKS': | ||
| case 'REPLACE_BLOCKS': | ||
| return defaultValue; |
Member
Author
There was a problem hiding this comment.
Note that this is no longer needed. The inserter state will be overwritten when the inserter closes or reopens.
|
Size Change: +87 B (0%) Total Size: 1.47 MB
ℹ️ View Unchanged
|
ellatrix
commented
Mar 26, 2021
| const onBrowseAll = () => { | ||
| __unstableSetInsertionPoint( rootClientId, blockIndex ); | ||
| setInserterIsOpened( true ); | ||
| setInserterIsOpened( { rootClientId, blockIndex } ); |
Member
Author
There was a problem hiding this comment.
Worth noting that this is an experimental API (getSettings().__experimentalSetIsInserterOpened).
youknowriad
reviewed
Mar 29, 2021
Member
|
From memory there's a lot of edge cases here to do with inner blocks and different editor contexts (post editor, site editor, widgets editor). I think that the testing steps I wrote in #26443 covers them. |
ellatrix
force-pushed
the
try/simpler-insertion-point
branch
2 times, most recently
from
1c0334e to
5be76d8
Compare
ellatrix
commented
Apr 21, 2021
| // the insertion point can work as expected | ||
| const onBrowseAll = () => { | ||
| __unstableSetInsertionPoint( rootClientId, blockIndex ); | ||
| setInserterIsOpened( true ); |
Member
Author
There was a problem hiding this comment.
I found it also strange that we'd have to set the insertion point, while no insertion point should show. I guess that's why the show/hide actions were added.
Member
Author
|
All tests are passing. I also tested the steps in #26443 and everything works fine. @youknowriad @noisysocks @talldan Could you have another look at this? |
ellatrix
commented
Apr 21, 2021
| if ( insertionIndex ) { | ||
| // Insert into a specific index. | ||
| _destinationIndex = insertionIndex; | ||
| } else if ( clientId ) { |
Member
Author
There was a problem hiding this comment.
I think at some point it would be nice to clean this up and remove isAppender and clientID in favour of insertionIndex.
youknowriad
reviewed
Apr 22, 2021
| return <InserterSidebar />; | ||
| return ( | ||
| <InserterSidebar | ||
| rootClientId={ isInserterOpen?.rootClientId } |
Contributor
There was a problem hiding this comment.
It seems weird that isInserterOpen is not a boolean. Can we rename the selectors? Are these stable?
Member
Author
There was a problem hiding this comment.
The setting (action) is exposed as experimental (__experimentalSetIsInserterOpened), but the selector was not. I created a separate experimental selector to get the insertion point and make sure that the isInserterOpen selector remains boolean.
youknowriad
approved these changes
Apr 22, 2021
Contributor
youknowriad
left a comment
youknowriad
left a comment
There was a problem hiding this comment.
Other than the naming thing, this seems to work well, I tried all the combination I can think of and it works properly.
7 tasks
ellatrix
requested review from
adamziel,
draganescu,
kevin940726 and
tellthemachines
as code owners
ellatrix
force-pushed
the
try/simpler-insertion-point
branch
from
4a329a6 to
98bb5aa
Compare
7 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.
Description
Blocking #29933 and #30285.
This PR seeks to undo some of the complexity introduced in #26443 for the insertion point. The insertion point used to be a simple visible indicator for where block will be inserted/dropped etc., but now it is also used for passing context to the main/library inserter, even when the insertion point is not visible.
This new behaviour is getting in the way when trying to use the insertion point action for other things like drop and the in-between inserter. Showing an insertion point at a different position will result in the loss of context for the main block library.
The solution I'm proposing is to pass the position data directly to the main library inserter when opening it.
It's worth noting that there's already a precedent for passing an index to the experimental
<Library>component, which we're reusing here.gutenberg/packages/edit-widgets/src/components/layout/interface.js
Lines 90 to 94 in 3da717b
How has this been tested?
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).