Update last inserted block state to track multiple blocks #46885
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
commented
Jan 4, 2023
| @@ -2647,7 +2647,7 @@ export const __experimentalGetActiveBlockIdByBlockNames = createSelector( | |||
| export function wasBlockJustInserted( state, clientId, source ) { | |||
| const { lastBlockInserted } = state; | |||
| return ( | |||
| lastBlockInserted.clientId === clientId && | |||
| lastBlockInserted.clientIds?.includes( clientId ) && | |||
There was a problem hiding this comment.
The idea here is to preserve the intent of the existing selector as if the block has just been inserted then it will stil be a valid result.
getdave
commented
Jan 4, 2023
| export function __experimentalGetLastInsertedBlockClientId( state ) { | ||
| return ( | ||
| state?.lastBlockInserted?.clientIds?.length && | ||
| state?.lastBlockInserted?.clientIds[ 0 ] |
There was a problem hiding this comment.
We could update this selector to return all the clientIds and allow the user to deal with which one they want to count as the "last" or "latest".
So if there is a single block you'd then consume it thus
const lastInsertedBlockClientId = __experimentalGetLastInsertedBlockClientIds()[0]
There was a problem hiding this comment.
I think we should return the array.
There was a problem hiding this comment.
Agreed, picking the first one is very arbitrary
There was a problem hiding this comment.
Excellent. We have a consensus. So I assume we:
- rename the selector to
__experimentalGetLastInsertedBlockClientIds(plural) - deprecate the original selector
There was a problem hiding this comment.
I'm going to follow the guidance and advice in the docs
There was a problem hiding this comment.
Ok I have now:
- deprecated the original selector (this is why it should have been experimental 🤦 )
- add a new selector which returns an array
I still don't know if the new selector should be experimental. @draganescu felt there was no need as it was in use by the offcanvas. But that itself is an experiment so ... 🤷♂️ Opinions appreciated.
There was a problem hiding this comment.
I vote we keep it experimental for now.
getdave
added
[Type] Regression
getdave
changed the title
|
Size Change: +263 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
getdave
requested review from
ellatrix,
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
I think these updates make sense. I don't think the selector should be experimental since it will be used by the Link UI. |
getdave
requested review from
draganescu,
scruffian,
MaggieCabrera and
ajlende
getdave
removed
the
[Type] Regression
draganescu
approved these changes
Jan 4, 2023
There was a problem hiding this comment.
I think the updates here make sense. The lastBlockInserted reducer is surprising now by selecting the "first" from a list of inserted blocks which could be very well not the last inserted.
My one nit is getLastInsertedBlockClientId should not be experimental because it is very simple and it will be used by Link UI right away.
I think you mean the selector? The reducer now tracks all blocks not just the first one. I kept the selector "as is" but we could easily change it to return all blocks. What do you think @draganescu? |
|
I pushed a fix to rename getLastInsertedBlockClientId to __experimentalGetLastInsertedBlockClientId as I was getting a JS error. |
youknowriad
requested changes
Jan 4, 2023
| } = select( blockEditorStore ); | ||
| return { | ||
| blockMovingClientId: hasBlockMovingClientId(), | ||
| selectedBlockInBlockEditor: getSelectedBlockClientId(), | ||
| lastInsertedBlockClientId: getLastInsertedBlockClientId(), | ||
| lastInsertedBlockClientId: | ||
| __experimentalGetLastInsertedBlockClientId(), |
There was a problem hiding this comment.
Did we merge the "@wordpress/experiments" PR yet, in which case prefixing is considered like an outdated approach?
There was a problem hiding this comment.
Maybe @adamziel would know the best approach to introduce an experimental selector right now?
There was a problem hiding this comment.
Yes that's a good point. It might be worth a separate PR to make it experimental and then rebase this PR.
There was a problem hiding this comment.
Due to the flux in the API since introduced, I've followed up and made this a private selector
| export function __experimentalGetLastInsertedBlockClientId( state ) { | ||
| return ( | ||
| state?.lastBlockInserted?.clientIds?.length && | ||
| state?.lastBlockInserted?.clientIds[ 0 ] |
There was a problem hiding this comment.
Agreed, picking the first one is very arbitrary
getdave
commented
Jan 5, 2023
| deprecated( | ||
| 'wp.data.select( "core/block-editor" ).getLastInsertedBlockClientId', | ||
| { | ||
| since: '15.0', |
There was a problem hiding this comment.
Can I get a confidence check that this is the correct release?
There was a problem hiding this comment.
14.9 was the last release so I believe so.
youknowriad
reviewed
Jan 5, 2023
As per @youknowriad advice in #46885 (comment)
youknowriad
approved these changes
Jan 5, 2023
bph
added
the
Needs Dev Note
|
Added the "Needs Dev Note" label to put in our our radar for WordPress 6.2 Fieldguide
... should it make it into this release. |
|
@getdave to make this selector experimental with the new lock()/unlock() API you'll need to:
// The `lock` function is exported from the internal experiments.js file where
// the opt-in function was called.
import { unlock } from './experiments';
import { getLastInsertedBlocksClientIds, ...selectors } from './selectors';
export const store = registerStore(/* ... */);
// Attach a private action to the exported store:
unlock( store ).registerPrivateSelector({
getLastInsertedBlocksClientIds
} );
import { unlock } from '../../experiments';
const {
hasBlockMovingClientId,
getSelectedBlockClientId,
getLastInsertedBlockClientId,
getLastInsertedBlocksClientIds,
} = unlock( select( blockEditorStore ) ); |
|
This selector will be private #47638 |
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?
Companion/sub PR to #46857.
Currently the last inserted block is tracked in state as a single clientId. However this only accounts for the scenario where a single block has been inserted.
This PR updates and addresses this concern by changing the state to track all inserted blocks and also providing new selectors to access this new state shape (whilst deprecating existing ones).
Why?
Currently the state only handle single blocks. It does not account for situations such as:
REPLACE_type actions which may still "insert" new blocks but may provide multiple blocks.INSERT_BLOCKSaction which may also provide multiple blocksThis is important as sometimes it's important to know when
REPLACE_type actions have caused new blocks to be inserted.By altering the reducer state we can track this whilst still preserving the behaviour of the original selectors.
How?
This PR:
getLastInsertedBlockClientIdselector which returned a single block only and is thus no longer suitable for accessing the state in an non-arbitrary way.getLastInsertedBlocksClientIdswhich returns an array of block clientIds.REPLACE_actions in the reducer to ensure these are also tracked as potential "insertions" of blocksTesting Instructions
Tests:
npm run test:unit packages/block-editor/src/store/test/selectors.jsnpm run test:unit packages/block-editor/src/store/test/reducer.jsYou can also add some blocks in a new Post and then run:
...and check that you see an array of blocks.
Also try adding some blocks, transforming into a quote and then unwrapping the quote back to individual blocks. Then run the selector above. You should see multiple blocks returned.
#### Check Depreciation Notice
Run
Check that the deprecation notice in console is correct.
Testing Instructions for Keyboard
Screenshots or screencast