Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move insertionPoint state to block-editor store/rename existing insertionPoint to insertionCue #65098

Merged
merged 25 commits into from
Sep 26, 2024
Merged

Move insertionPoint state to block-editor store/rename existing insertionPoint to insertionCue #65098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
3e599d2
Use index instead of insertionIndex
jeryj Sep 5, 2024
458c376
Revert "Use index instead of insertionIndex"
jeryj Sep 5, 2024
aae2353
Move/Add inserterInsertionPoint to block editor
jeryj Sep 6, 2024
269fe53
Update the docs of setIsInserterOpened
jeryj Sep 9, 2024
b17df45
Remove unnecessary props since they won't be set
jeryj Sep 9, 2024
2462257
Discourage use of rootIndex and insertionIndex on setIsInserterOpened
jeryj Sep 9, 2024
b06bddd
Remove note
jeryj Sep 9, 2024
8136045
Use setInserterInsertionPoint in quick inserter
jeryj Sep 9, 2024
a551d83
Insert in correct location for root insertions
jeryj Sep 10, 2024
a1765fe
Add store tests and improve docs and default state
jeryj Sep 10, 2024
5959cd8
Use inserterInsertionPoint for zoom out inserters and zoom out separator
jeryj Sep 11, 2024
3705906
Remove blockInsertionPointVisible from zoom out mode inserters
jeryj Sep 11, 2024
72e8309
Fix dragging vertical displacement on insertion points
jeryj Sep 11, 2024
355a061
Fix inserting at 0 index on zoom out mode
jeryj Sep 11, 2024
136c9a3
Fix insertion point at root level
jeryj Sep 11, 2024
867641e
Deprecate rootClientId and insertionIndex on setIsInserterOpened
jeryj Sep 12, 2024
bfff2d4
Renaming Insertion API for clarity
jeryj Sep 13, 2024
4f262d1
Catch missed renamings
jeryj Sep 13, 2024
24a2aec
More renamings and clarification
jeryj Sep 13, 2024
8d6410c
Fix logic and missing private function in editor store
jeryj Sep 25, 2024
28e1596
Fallback to using setInserterIsOpened if already in use
jeryj Sep 25, 2024
a21fe77
Move actions test within reducer to actions test and test with select…
jeryj Sep 26, 2024
98cf472
Remove unncessary return on dispatch
jeryj Sep 26, 2024
d037772
Update block editor selector state tests from insertionPoint to inser…
jeryj Sep 26, 2024
8c48524
Move code back to previous location
jeryj Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/reference-guides/data/data-core-block-editor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ _Returns_

### getBlockInsertionPoint

Returns the insertion point, the index at which the new inserted block would be placed. Defaults to the last index.
Returns the location of the insertion cue. Defaults to the last index.

_Parameters_

Expand Down Expand Up @@ -982,7 +982,7 @@ _Returns_

### isBlockInsertionPointVisible

Returns true if we should show the block insertion point.
Returns true if the block insertion point is visible.

_Parameters_

Expand Down
4 changes: 4 additions & 0 deletions docs/reference-guides/data/data-core-editor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1422,6 +1422,10 @@ _Parameters_
- _value_ `boolean|Object`: Whether the inserter should be opened (true) or closed (false). To specify an insertion point, use an object.
- _value.rootClientId_ `string`: The root client ID to insert at.
- _value.insertionIndex_ `number`: The index to insert at.
- _value.filterValue_ `string`: A query to filter the inserter results.
- _value.onSelect_ `Function`: A callback when an item is selected.
- _value.tab_ `string`: The tab to open in the inserter.
- _value.category_ `string`: The category to initialize in the inserter.
Comment on lines +1425 to +1428
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not new, just adding the documentation for things already in use.


_Returns_

Expand Down
32 changes: 24 additions & 8 deletions packages/block-editor/src/components/block-list/zoom-out-separator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,16 @@ export function ZoomOutSeparator( {
const {
sectionRootClientId,
sectionClientIds,
blockInsertionPoint,
insertionPoint,
blockInsertionPointVisible,
blockInsertionPoint,
} = useSelect( ( select ) => {
const {
getBlockInsertionPoint,
getInsertionPoint,
getBlockOrder,
isBlockInsertionPointVisible,
getSectionRootClientId,
isBlockInsertionPointVisible,
getBlockInsertionPoint,
} = unlock( select( blockEditorStore ) );

const root = getSectionRootClientId();
Expand All @@ -45,6 +47,7 @@ export function ZoomOutSeparator( {
sectionRootClientId: root,
sectionClientIds: sectionRootClientIds,
blockOrder: getBlockOrder( root ),
insertionPoint: getInsertionPoint(),
blockInsertionPoint: getBlockInsertionPoint(),
blockInsertionPointVisible: isBlockInsertionPointVisible(),
};
Expand All @@ -67,17 +70,30 @@ export function ZoomOutSeparator( {
return null;
}

const hasTopInsertionPoint =
insertionPoint?.index === 0 &&
clientId === sectionClientIds[ insertionPoint.index ];
const hasBottomInsertionPoint =
insertionPoint &&
insertionPoint.hasOwnProperty( 'index' ) &&
clientId === sectionClientIds[ insertionPoint.index - 1 ];
// We want to show the zoom out separator in either of these conditions:
// 1. If the inserter has an insertion index set
// 2. We are dragging a pattern over an insertion point
if ( position === 'top' ) {
isVisible =
blockInsertionPointVisible &&
blockInsertionPoint.index === 0 &&
clientId === sectionClientIds[ blockInsertionPoint.index ];
hasTopInsertionPoint ||
( blockInsertionPointVisible &&
blockInsertionPoint.index === 0 &&
clientId === sectionClientIds[ blockInsertionPoint.index ] );
}

if ( position === 'bottom' ) {
isVisible =
blockInsertionPointVisible &&
clientId === sectionClientIds[ blockInsertionPoint.index - 1 ];
hasBottomInsertionPoint ||
( blockInsertionPointVisible &&
clientId ===
sectionClientIds[ blockInsertionPoint.index - 1 ] );
}

return (
Expand Down
13 changes: 7 additions & 6 deletions packages/block-editor/src/components/block-tools/zoom-out-mode-inserters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ function ZoomOutModeInserters() {
const [ isReady, setIsReady ] = useState( false );
const {
hasSelection,
blockInsertionPoint,
insertionPoint,
blockOrder,
blockInsertionPointVisible,
setInserterIsOpened,
Expand All @@ -26,20 +26,20 @@ function ZoomOutModeInserters() {
} = useSelect( ( select ) => {
const {
getSettings,
getBlockInsertionPoint,
getInsertionPoint,
getBlockOrder,
getSelectionStart,
getSelectedBlockClientId,
getHoveredBlockClientId,
isBlockInsertionPointVisible,
getSectionRootClientId,
isBlockInsertionPointVisible,
} = unlock( select( blockEditorStore ) );

const root = getSectionRootClientId();

return {
hasSelection: !! getSelectionStart().clientId,
blockInsertionPoint: getBlockInsertionPoint(),
insertionPoint: getInsertionPoint(),
blockOrder: getBlockOrder( root ),
blockInsertionPointVisible: isBlockInsertionPointVisible(),
sectionRootClientId: root,
Expand All @@ -50,7 +50,8 @@ function ZoomOutModeInserters() {
};
}, [] );

const { showInsertionPoint } = useDispatch( blockEditorStore );
// eslint-disable-next-line @wordpress/no-unused-vars-before-return
const { showInsertionPoint } = unlock( useDispatch( blockEditorStore ) );

// Defer the initial rendering to avoid the jumps due to the animation.
useEffect( () => {
Expand All @@ -68,7 +69,7 @@ function ZoomOutModeInserters() {

return [ undefined, ...blockOrder ].map( ( clientId, index ) => {
const shouldRenderInsertionPoint =
blockInsertionPointVisible && blockInsertionPoint.index === index;
blockInsertionPointVisible && insertionPoint?.index === index;

const previousClientId = clientId;
const nextClientId = blockOrder[ index ];
Expand Down
13 changes: 11 additions & 2 deletions packages/block-editor/src/components/inserter/hooks/use-insertion-point.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,15 +83,24 @@ function useInsertionPoint( {
getBlockRootClientId,
getBlockIndex,
getBlockOrder,
} = select( blockEditorStore );
getInsertionPoint,
} = unlock( select( blockEditorStore ) );
const selectedBlockClientId = getSelectedBlockClientId();

let _destinationRootClientId = rootClientId;
let _destinationIndex;
const insertionPoint = getInsertionPoint();

if ( insertionIndex !== undefined ) {
// Insert into a specific index.
_destinationIndex = insertionIndex;
} else if (
insertionPoint &&
insertionPoint.hasOwnProperty( 'index' )
) {
_destinationRootClientId = insertionPoint?.rootClientId
? insertionPoint.rootClientId
: rootClientId;
_destinationIndex = insertionPoint.index;
} else if ( clientId ) {
// Insert after a specific client ID.
_destinationIndex = getBlockIndex( clientId );
Expand Down
4 changes: 2 additions & 2 deletions packages/block-editor/src/components/inserter/quick-inserter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,10 @@ export default function QuickInserter( {
// the insertion point can work as expected.
const onBrowseAll = () => {
setInserterIsOpened( {
rootClientId,
insertionIndex,
filterValue,
onSelect,
rootClientId,
insertionIndex,
} );
};

Expand Down
14 changes: 14 additions & 0 deletions packages/block-editor/src/store/private-actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,20 @@ export function expandBlock( clientId ) {
};
}

/**
* @param {Object} value
* @param {string} value.rootClientId The root client ID to insert at.
* @param {number} value.index The index to insert at.
*
* @return {Object} Action object.
*/
export function setInsertionPoint( value ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this different than showInsertionPoint?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so If I'm understand properly we have:

  • showInsertionPoint which is the exact location where the block will be inserted. So the true insertion point.
  • and we have setInsertionPoint which is about the top level "inserter" component. IT's about setting the "default" insertion point (if blocks are not allowed to be inserted there, we compute the true insertion point and show it using showInsertionPoint)

is that correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarifying my understanding more:

The "inserter insertion point" state was being set in the "editor" package because the "Inserter" component has a "rootClientId" and "clientId" props, so this one is controlled externally, I guess the idea is that at the same time, you can potentially render different inserters with different default insertion points. The current PR seems to move the state to "block-editor" but the props remain so it's still "externally" controlled right? So why the move to "block-editor" state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this different than showInsertionPoint?
showInsertionPoint makes the insertion cue visible. It is not code-wise related to the insertion point, just a visual indicator.

showInsertionPoint which is the exact location where the block will be inserted. So the true insertion point.

No, this only shows where the blue line will get shown (what I will now refer to as the insertion cue for clarity). We don't have a true insertion point at the moment other than passing an insertion point to useInsertionPoint (what the sidebar inserter was doing) or via the new setInsertionPoint added in this PR.

The current PR seems to move the state to "block-editor" but the props remain so it's still "externally" controlled right? So why the move to "block-editor" state.

Exactly. No props were removed and a manually passed insertion point to the inserter will still be respected. We needed to move it to the block-editor state to clear the insertion point when a block is selected.

This reverted PR demos more about why we need this. Code-wise, the inserter was built with a static insertion point in mind. Now the inserter can be open with selection on the block canvas happening, meaning the insertion point can be updated while the inserter is still open.

The problem with the PR above is that it relied on getBlockInsertionPoint which updates constantly on hover, so we had to revert it.

I wrote an overview of the Insertion API current state here: #65290

return {
type: 'SET_INSERTION_POINT',
value,
};
}

/**
* Temporarily modify/unlock the content-only block for editions.
*
Expand Down
10 changes: 10 additions & 0 deletions packages/block-editor/src/store/private-selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,3 +698,13 @@ export function getClosestAllowedInsertionPointForPattern(
const names = getGrammar( pattern ).map( ( { blockName: name } ) => name );
return getClosestAllowedInsertionPoint( state, names, clientId );
}

/**
* Where the point where the next block will be inserted into.
*
* @param {Object} state
* @return {Object} where the insertion point in the block editor is or null if none is set.
*/
export function getInsertionPoint( state ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this different than getBlockInsertionPoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getBlockInsertionPoint is really about the blue insertion cue that gets shown. getBlockInsertionPoint is constantly updated based on where you're hovering the canvas. Maybe originally getBlockInsertionPoint was about where the insertion would happen, but its code now is tied to where the insertion cue shows.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference for you between "insertion cue" and "insertion position". If we're showing a different insertion cue than the actual insertion point, then we have problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is that one is an index and the other one is an UI element?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both are state for me. The UI element is just a reflexion of a state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it gets tricky when you include drag and drop. You’re showing an insertion cue at a location that may be different than what is set via selection.

If you update that point on hover via drag and drop, this is the correct behavior for the insertion cue/insertion point, but a lot of the things built off using it were not intended to be updating their insertion point that often.

So, I think insertion cue of where insertion will happen and should rely on insertion point if set, but the insertion cue should be able to show separate from the insertion point on instances like drag and drop. Hovering the canvas should not be updating the insertion point, it should be updating a cue about where you might be able to insert.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I think insertion cue of where insertion will happen and should rely on insertion point if set, but the insertion cue should be able to show separate from the insertion point on instances like drag and drop. Hovering the canvas should not be updating the insertion point, it should be updating a cue about where you might be able to insert.

I agree with this, but it doesn't really explain the PR to me. we were using showInsertionPoint for me before this PR to do this and this was already different than the "default" insertion point used in the inserter component.

So I'm still not clear what this PR is trying to accomplish aside moving things around and potentially making them more complicated.

return state.insertionPoint;
}
24 changes: 22 additions & 2 deletions packages/block-editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1601,7 +1601,7 @@ export function blocksMode( state = {}, action ) {
*
* @return {Object} Updated state.
*/
export function insertionPoint( state = null, action ) {
export function insertionCue( state = null, action ) {
switch ( action.type ) {
case 'SHOW_INSERTION_POINT': {
const {
Expand Down Expand Up @@ -2066,7 +2066,7 @@ export function hoveredBlockClientId( state = false, action ) {
* @param {boolean} state Current state.
* @param {Object} action Dispatched action.
*
* @return {boolean} Updated state.
* @return {number} Updated state.
*/
export function zoomLevel( state = 100, action ) {
switch ( action.type ) {
Expand All @@ -2079,6 +2079,25 @@ export function zoomLevel( state = 100, action ) {
return state;
}

/**
* Reducer setting the insertion point
*
* @param {boolean} state Current state.
* @param {Object} action Dispatched action.
*
* @return {Object} Updated state.
*/
export function insertionPoint( state = null, action ) {
switch ( action.type ) {
case 'SET_INSERTION_POINT':
return action.value;
case 'SELECT_BLOCK':
return null;
Comment on lines +2094 to +2095
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear the insertion point after a new block selection.

Copy link
Contributor

@scruffian scruffian Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing, the vertical displacement is still visible after selecting a different block.

}

return state;
}

const combinedReducers = combineReducers( {
blocks,
isDragging,
Expand All @@ -2092,6 +2111,7 @@ const combinedReducers = combineReducers( {
blocksMode,
blockListSettings,
insertionPoint,
insertionCue,
template,
settings,
preferences,
Expand Down
15 changes: 7 additions & 8 deletions packages/block-editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1454,8 +1454,7 @@ export function isCaretWithinFormattedText() {
}

/**
* Returns the insertion point, the index at which the new inserted block would
* be placed. Defaults to the last index.
* Returns the location of the insertion cue. Defaults to the last index.
*
* @param {Object} state Editor state.
*
Expand All @@ -1466,11 +1465,11 @@ export const getBlockInsertionPoint = createSelector(
let rootClientId, index;

const {
insertionPoint,
insertionCue,
selection: { selectionEnd },
} = state;
if ( insertionPoint !== null ) {
return insertionPoint;
if ( insertionCue !== null ) {
return insertionCue;
}

const { clientId } = selectionEnd;
Expand All @@ -1485,22 +1484,22 @@ export const getBlockInsertionPoint = createSelector(
return { rootClientId, index };
},
( state ) => [
state.insertionPoint,
state.insertionCue,
state.selection.selectionEnd.clientId,
state.blocks.parents,
state.blocks.order,
]
);

/**
* Returns true if we should show the block insertion point.
* Returns true if the block insertion point is visible.
*
* @param {Object} state Global application state.
*
* @return {?boolean} Whether the insertion point is visible or not.
*/
export function isBlockInsertionPointVisible( state ) {
return state.insertionPoint !== null;
return state.insertionCue !== null;
}

/**
Expand Down
15 changes: 15 additions & 0 deletions packages/block-editor/src/store/test/private-actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
showBlockInterface,
expandBlock,
__experimentalUpdateSettings,
setInsertionPoint,
setOpenedBlockSettingsMenu,
startDragging,
stopDragging,
Expand Down Expand Up @@ -123,4 +124,18 @@ describe( 'private actions', () => {
} );
} );
} );

describe( 'setInsertionPoint', () => {
it( 'should return the SET_INSERTION_POINT action', () => {
expect(
setInsertionPoint( {
rootClientId: '',
index: '123',
} )
).toEqual( {
type: 'SET_INSERTION_POINT',
value: { rootClientId: '', index: '123' },
} );
} );
} );
Comment on lines +128 to +140
Copy link
Contributor

@ajlende ajlende Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test and all the other tests in this file aren't doing anything useful.

The important thing for actions is that reducers do things based on them and the selectors read out a specific value. The parts of a store shouldn't be tested in isolation like this.

It looks like you're trying to fix the breaking change that may have happened some time earlier, but updating the test and documentation isn't the way to fix it. The prior logic needs to be restored.

If the tests were for the store as a whole, maybe this would have been caught.

Also, to be clear, I'm suggesting moving the tests, documentation, and fixing the setIsInserterOpened regression described in your comment into a different PR.

} );
Loading
Loading