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.

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

Allow using multiple controlled inner blocks refering to the same entity #27885

Merged
merged 3 commits into from
Dec 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@ describe( 'useBlockSync hook', () => {
expect( onChange ).not.toHaveBeenCalled();
expect( onInput ).not.toHaveBeenCalled();
expect( resetBlocks ).not.toHaveBeenCalled();
expect( replaceInnerBlocks ).toHaveBeenCalledWith( 'test', testBlocks );
// We can't check the args because the blocks are cloned.
expect( replaceInnerBlocks ).toHaveBeenCalled();
} );

it( 'does not add the controlled blocks to the block-editor store if the store already contains them', async () => {
Expand Down Expand Up @@ -200,7 +201,6 @@ describe( 'useBlockSync hook', () => {
it( 'calls onInput when a non-persistent block change occurs', async () => {
const onChange = jest.fn();
const onInput = jest.fn();

const value1 = [
{ clientId: 'a', innerBlocks: [], attributes: { foo: 1 } },
];
Expand Down
77 changes: 44 additions & 33 deletions packages/block-editor/src/components/provider/use-block-sync.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { last, noop } from 'lodash';
*/
import { useEffect, useRef } from '@wordpress/element';
import { useRegistry } from '@wordpress/data';
import { cloneBlock } from '@wordpress/blocks';

/**
* A function to call when the block value has been updated in the block-editor
Expand Down Expand Up @@ -83,6 +84,7 @@ export default function useBlockSync( {
const { getBlockName, getBlocks } = registry.select( 'core/block-editor' );

const pendingChanges = useRef( { incoming: null, outgoing: [] } );
const subscribed = useRef( false );

const setControlledBlocks = () => {
if ( ! controlledBlocks ) {
Expand All @@ -96,8 +98,17 @@ export default function useBlockSync( {
if ( clientId ) {
setHasControlledInnerBlocks( clientId, true );
__unstableMarkNextChangeAsNotPersistent();
replaceInnerBlocks( clientId, controlledBlocks );
const storeBlocks = controlledBlocks.map( ( block ) =>
cloneBlock( block )
);
if ( subscribed.current ) {
pendingChanges.current.incoming = storeBlocks;
}
replaceInnerBlocks( clientId, storeBlocks );
} else {
if ( subscribed.current ) {
pendingChanges.current.incoming = controlledBlocks;
}
resetBlocks( controlledBlocks );
}
};
Expand All @@ -113,6 +124,37 @@ export default function useBlockSync( {
onChangeRef.current = onChange;
}, [ onInput, onChange ] );

// Determine if blocks need to be reset when they change.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes after this line are not necessary (just reordering the hooks). I believe this order makes more sense. "External" then "Internal" changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it seems the reordering has an effect, without it, this PR doesn't work :)

useEffect( () => {
if ( pendingChanges.current.outgoing.includes( controlledBlocks ) ) {
// Skip block reset if the value matches expected outbound sync
// triggered by this component by a preceding change detection.
// Only skip if the value matches expectation, since a reset should
// still occur if the value is modified (not equal by reference),
// to allow that the consumer may apply modifications to reflect
// back on the editor.
if (
last( pendingChanges.current.outgoing ) === controlledBlocks
) {
pendingChanges.current.outgoing = [];
}
} else if ( getBlocks( clientId ) !== controlledBlocks ) {
// Reset changing value in all other cases than the sync described
// above. Since this can be reached in an update following an out-
// bound sync, unset the outbound value to avoid considering it in
// subsequent renders.
pendingChanges.current.outgoing = [];
setControlledBlocks();

if ( controlledSelectionStart && controlledSelectionEnd ) {
resetSelection(
controlledSelectionStart,
controlledSelectionEnd
);
}
}
}, [ controlledBlocks, clientId ] );

useEffect( () => {
const {
getSelectionStart,
Expand All @@ -125,6 +167,7 @@ export default function useBlockSync( {
let isPersistent = isLastBlockChangePersistent();
let previousAreBlocksDifferent = false;

subscribed.current = true;
const unsubscribe = registry.subscribe( () => {
// Sometimes, when changing block lists, lingering subscriptions
// might trigger before they are cleaned up. If the block for which
Expand Down Expand Up @@ -184,36 +227,4 @@ export default function useBlockSync( {

return () => unsubscribe();
}, [ registry, clientId ] );

// Determine if blocks need to be reset when they change.
useEffect( () => {
if ( pendingChanges.current.outgoing.includes( controlledBlocks ) ) {
// Skip block reset if the value matches expected outbound sync
// triggered by this component by a preceding change detection.
// Only skip if the value matches expectation, since a reset should
// still occur if the value is modified (not equal by reference),
// to allow that the consumer may apply modifications to reflect
// back on the editor.
if (
last( pendingChanges.current.outgoing ) === controlledBlocks
) {
pendingChanges.current.outgoing = [];
}
} else if ( getBlocks( clientId ) !== controlledBlocks ) {
// Reset changing value in all other cases than the sync described
// above. Since this can be reached in an update following an out-
// bound sync, unset the outbound value to avoid considering it in
// subsequent renders.
pendingChanges.current.outgoing = [];
pendingChanges.current.incoming = controlledBlocks;
setControlledBlocks();

if ( controlledSelectionStart && controlledSelectionEnd ) {
resetSelection(
controlledSelectionStart,
controlledSelectionEnd
);
}
}
}, [ controlledBlocks, clientId ] );
}