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

Fix: deleting the last block triggers a focus loss. #14189

Merged
merged 4 commits into from
Mar 1, 2019
Merged

Fix: deleting the last block triggers a focus loss. #14189

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b4bbd1c
Fix: deleting the last block triggers a focus loss.
jorgefilipecosta Mar 1, 2019
94f8c9d
Update packages/block-editor/src/store/test/actions.js
aduth Mar 1, 2019
19b0998
Block Editor: Yield remove actions directly
aduth Mar 1, 2019
3c5cd95
Editor: Clarify behavior of insertDefaultBlock on remove last
aduth Mar 1, 2019
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
10 changes: 10 additions & 0 deletions docs/designers-developers/developers/data/data-core-block-editor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,6 +958,16 @@ Returns an action object used in signalling that two blocks should be merged
* firstBlockClientId: Client ID of the first block to merge.
* secondBlockClientId: Client ID of the second block to merge.

### __internalRemoveBlocksPure
Copy link
Member

Choose a reason for hiding this comment

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

This naming convention is not standard.

Copy link
Member Author

Choose a reason for hiding this comment

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

Normally for experimental functions we use __experimental I followed something similar and used __internal as a prefix, I was not aware we have a were following a standard for this. What you think would be the best prefix for a case like this?

Copy link
Member

Choose a reason for hiding this comment

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

They're documented here:

https://github.com/WordPress/gutenberg/blob/535e0752051957f3e6ab7aca2e71e99ceffc9a6f/docs/contributors/coding-guidelines.md#experimental-apis

I raise as a concern of: Introducing new vocabulary contributes to lack of cohesiveness and understanding when not consciously agreed upon, documented, and used consistency.

For example, it's not clear to me what differentiates an "internal" and "unstable" function. The problem admittedly already exists to an extent between unstable and experimental, and its confusion has prompted more than a few conversations.

Copy link
Member

Choose a reason for hiding this comment

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

What you think would be the best prefix for a case like this?

I'd rather see the function not exist at all, if possible, per the other conversation thread. If needing to act before / after a dispatch takes effect is an issue, it's quite unlikely that this will be isolated, and this pattern does not seem like one which would scale well.


Returns action objects used in signalling that the blocks corresponding to
the set of specified client IDs are to be removed.
This action does not trigger any required side effects and it is not recommended for public usage.

*Parameters*

* clientIds: Client IDs of blocks to remove.

### removeBlocks

Yields action objects used in signalling that the blocks corresponding to
Expand Down
40 changes: 35 additions & 5 deletions packages/block-editor/src/store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { getDefaultBlockName, createBlock } from '@wordpress/blocks';
/**
* Internal dependencies
*/
import { select } from './controls';
import { select, dispatch } from './controls';

/**
* Returns an action object used in signalling that blocks state should be
Expand Down Expand Up @@ -374,6 +374,23 @@ export function mergeBlocks( firstBlockClientId, secondBlockClientId ) {
};
}

/**
* Returns action objects used in signalling that the blocks corresponding to
* the set of specified client IDs are to be removed.
* This action does not trigger any required side effects and it is not recommended for public usage.
*
* @param {string|string[]} clientIds Client IDs of blocks to remove.
*
* @return {Object} Action object.
*
*/
export function __internalRemoveBlocksPure( clientIds ) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why this needs to exist. What was the issue with the previous yield { type: 'REMOVE_BLOCKS' } ?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we do that the yield select that appears after will return the value before the store is changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

This solution seems a little bit hacky, I'm probably missing something but it seemed the only way I could select from the store after the action changed the store. If there is a simpler method that I'm missing I will gladly change the code.

Copy link
Member

Choose a reason for hiding this comment

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

If we do that the yield select that appears after will return the value before the store is changed.

Then if we're at the moment prior to removal, would it be enough to test that the count is === 1 (or I guess === clientIds.length), assuming that it'd become zero after the removal?

An issue with this may be that there's nothing to stop me from dispatching this action with a clientId which doesn't actually exist, at which point there's still one block after calling removeBlocks

Copy link
Member Author

@jorgefilipecosta jorgefilipecosta Mar 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Then if we're at the moment prior to removal, would it be enough to test that the count is === 1 (or I guess === clientIds.length), assuming that it'd become zero after the removal?

This logic would be more complex.
If I have a columns block with lots of blocks inside wp.data.select( 'core/block-editor' ).getBlockCount() === 1 because only columns block exists at the root level, I may remove one or more blocks inside the columns and getBlockCount() will continue to return 1.
We would need checks to understand if the block being removed is the one at the root level, in this case, this checks would not be very hard. But, my plan is to use this approach in other PR I have for the block replace case and there checks are even more complex.
Ideally, we would have a solution that allows side effects depending on the store before the store is changed and after the store is changed without checks, similar to what a middleware allows.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, we would have a solution that allows side effects depending on the store before the store is changed and after the store is changed without checks, similar to what a middleware allows.

Yeah, in fact, re-reading your original remark, this seems like buggy behavior to me. The result of select seems like it should be accurate at the point in time at which the control was yielded.

If we do that the yield select that appears after will return the value before the store is changed.

return {
type: 'REMOVE_BLOCKS',
clientIds,
};
}

/**
* Yields action objects used in signalling that the blocks corresponding to
* the set of specified client IDs are to be removed.
Expand All @@ -386,13 +403,26 @@ export function* removeBlocks( clientIds, selectPrevious = true ) {
clientIds = castArray( clientIds );

if ( selectPrevious ) {
yield selectPreviousBlock( clientIds[ 0 ] );
yield dispatch(
'core/block-editor',
'selectPreviousBlock',
clientIds[ 0 ]
);
}

yield {
type: 'REMOVE_BLOCKS',
yield dispatch(
'core/block-editor',
'__internalRemoveBlocksPure',
clientIds,
};
);

const count = yield select(
'core/block-editor',
'getBlockCount',
);
if ( count === 0 ) {
yield insertDefaultBlock();
}
}

/**
Expand Down
23 changes: 23 additions & 0 deletions packages/block-editor/src/store/controls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,33 @@ export function select( storeName, selectorName, ...args ) {
};
}

/**
* Dispatches a control action for triggering a registry dispatch.
*
* @param {string} storeKey
* @param {string} actionName
* @param {Array} args Arguments for the dispatch action.
*
* @return {Object} control descriptor.
*/
export function dispatch( storeKey, actionName, ...args ) {
return {
type: 'DISPATCH',
storeKey,
actionName,
args,
};
}

const controls = {
SELECT: createRegistryControl( ( registry ) => ( { storeName, selectorName, args } ) => {
return registry.select( storeName )[ selectorName ]( ...args );
} ),
DISPATCH: createRegistryControl(
( registry ) => ( { storeKey, actionName, args } ) => {
return registry.dispatch( storeKey )[ actionName ]( ...args );
}
),
};

export default controls;
3 changes: 0 additions & 3 deletions packages/block-editor/src/store/effects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,6 @@ export default {
RESET_BLOCKS: [
validateBlocksToTemplate,
],
REMOVE_BLOCKS: [
ensureDefaultBlock,
],
REPLACE_BLOCKS: [
ensureDefaultBlock,
],
Expand Down
72 changes: 56 additions & 16 deletions packages/block-editor/src/store/test/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {
updateBlockAttributes,
updateBlock,
selectBlock,
selectPreviousBlock,
startMultiSelect,
stopMultiSelect,
multiSelect,
Expand All @@ -27,8 +26,11 @@ import {
removeBlock,
toggleBlockMode,
updateBlockListSettings,
__internalRemoveBlocksPure,
} from '../actions';

jorgefilipecosta marked this conversation as resolved.
Show resolved Hide resolved
import { select, dispatch } from '../controls';

describe( 'actions', () => {
describe( 'resetBlocks', () => {
it( 'should return the RESET_BLOCKS actions', () => {
Expand Down Expand Up @@ -205,6 +207,21 @@ describe( 'actions', () => {
} );
} );

describe( '__internalRemoveBlocksPure', () => {
it( 'should return REMOVE_BLOCKS action', () => {
const clientIds = [ 'myclientid' ];

const action = __internalRemoveBlocksPure( clientIds );

expect( action ).toEqual(
{
type: 'REMOVE_BLOCKS',
clientIds,
},
);
} );
} );

describe( 'removeBlocks', () => {
it( 'should return REMOVE_BLOCKS action', () => {
const clientId = 'clientId';
Expand All @@ -213,11 +230,20 @@ describe( 'actions', () => {
const actions = Array.from( removeBlocks( clientIds ) );

expect( actions ).toEqual( [
selectPreviousBlock( clientId ),
{
type: 'REMOVE_BLOCKS',
clientIds,
},
dispatch(
'core/block-editor',
'selectPreviousBlock',
clientId
),
dispatch(
'core/block-editor',
'__internalRemoveBlocksPure',
[ clientId ],
),
select(
'core/block-editor',
'getBlockCount',
),
] );
} );
} );
Expand All @@ -229,24 +255,38 @@ describe( 'actions', () => {
const actions = Array.from( removeBlock( clientId ) );

expect( actions ).toEqual( [
selectPreviousBlock( clientId ),
{
type: 'REMOVE_BLOCKS',
clientIds: [ clientId ],
},
dispatch(
'core/block-editor',
'selectPreviousBlock',
clientId
),
dispatch(
'core/block-editor',
'__internalRemoveBlocksPure',
[ clientId ],
),
select(
'core/block-editor',
'getBlockCount',
),
] );
} );

it( 'should return REMOVE_BLOCKS action, opting out of remove previous', () => {
it( 'should return REMOVE_BLOCKS action, opting out of select previous', () => {
const clientId = 'myclientid';

const actions = Array.from( removeBlock( clientId, false ) );

expect( actions ).toEqual( [
{
type: 'REMOVE_BLOCKS',
clientIds: [ clientId ],
},
dispatch(
'core/block-editor',
'__internalRemoveBlocksPure',
[ clientId ],
),
select(
'core/block-editor',
'getBlockCount',
),
] );
} );
} );
Expand Down