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

onWriting Flow: Remove the provisional block behavior #8706

Merged
merged 2 commits into from
Aug 8, 2018
Merged

onWriting Flow: Remove the provisional block behavior #8706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3c05051
Writing Flow: Remove the provisional block behavior
youknowriad Aug 8, 2018
94d263a
Prefer unmodified default block terminology as it already exists in t…
youknowriad Aug 8, 2018
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
15 changes: 1 addition & 14 deletions docs/data/data-core-editor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# **core/editor**: The Editor’s Data

## Selectors
## Selectors

### hasEditorUndo

Expand Down Expand Up @@ -943,19 +943,6 @@ Returns true if the post is being published, or false otherwise.

Whether post is being published.

### getProvisionalBlockClientId

Returns the provisional block client ID, or null if there is no provisional
block.

*Parameters*

* state: Editor state.

*Returns*

Provisional block client ID, if set.

### isPermalinkEditable

Returns whether the permalink is editable or not.
Expand Down
5 changes: 1 addition & 4 deletions packages/editor/src/store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,10 +716,7 @@ export function convertBlockToReusable( clientId ) {
export function insertDefaultBlock( attributes, rootClientId, index ) {
const block = createBlock( getDefaultBlockName(), attributes );

return {
...insertBlock( block, index, rootClientId ),
isProvisional: true,
};
return insertBlock( block, index, rootClientId );
}

/**
Expand Down
26 changes: 0 additions & 26 deletions packages/editor/src/store/effects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import {
createWarningNotice,
insertBlock,
selectBlock,
removeBlock,
resetBlocks,
setTemplateValidity,
} from './actions';
Expand All @@ -37,9 +36,7 @@ import {
getBlockRootClientId,
getBlocks,
getPreviousBlockClientId,
getProvisionalBlockClientId,
getSelectedBlock,
isBlockSelected,
getTemplate,
getTemplateLock,
} from './selectors';
Expand All @@ -61,23 +58,6 @@ import {
AUTOSAVE_POST_NOTICE_ID,
} from './effects/posts';

/**
* Effect handler returning an action to remove the provisional block, if one
* is set.
*
* @param {Object} action Action object.
* @param {Object} store Data store instance.
*
* @return {?Object} Remove action, if provisional block is set.
*/
export function removeProvisionalBlock( action, store ) {
const state = store.getState();
const provisionalBlockClientId = getProvisionalBlockClientId( state );
if ( provisionalBlockClientId && ! isBlockSelected( state, provisionalBlockClientId ) ) {
return removeBlock( provisionalBlockClientId, false );
}
}

export default {
REQUEST_POST_UPDATE: ( action, store ) => {
requestPostUpdate( action, store );
Expand Down Expand Up @@ -244,12 +224,6 @@ export default {
}
},

CLEAR_SELECTED_BLOCK: removeProvisionalBlock,

SELECT_BLOCK: removeProvisionalBlock,

MULTI_SELECT: removeProvisionalBlock,

REMOVE_BLOCKS( action, { getState, dispatch } ) {
// if the action says previous block should not be selected don't do anything.
if ( ! action.selectPrevious ) {
Expand Down
44 changes: 0 additions & 44 deletions packages/editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {
omitBy,
keys,
isEqual,
includes,
overSome,
get,
} from 'lodash';
Expand Down Expand Up @@ -697,48 +696,6 @@ export function blockSelection( state = {
return state;
}

/**
* Reducer returning the client ID of the provisional block. A provisional
* block is one which is to be removed if it does not receive updates in the
* time until the next selection or block reset.
*
* @param {string} state Current state.
* @param {Object} action Dispatched action.
*
* @return {string} Updated state.
*/
export function provisionalBlockClientId( state = null, action ) {
switch ( action.type ) {
case 'INSERT_BLOCKS':
if ( action.isProvisional ) {
return first( action.blocks ).clientId;
}
break;

case 'RESET_BLOCKS':
return null;

case 'UPDATE_BLOCK_ATTRIBUTES':
case 'UPDATE_BLOCK':
case 'CONVERT_BLOCK_TO_REUSABLE':
const { clientId } = action;
if ( clientId === state ) {
return null;
}
break;

case 'REPLACE_BLOCKS':
case 'REMOVE_BLOCKS':
const { clientIds } = action;
if ( includes( clientIds, state ) ) {
return null;
}
break;
}

return state;
}

export function blocksMode( state = {}, action ) {
if ( action.type === 'TOGGLE_BLOCK_MODE' ) {
const { clientId } = action;
Expand Down Expand Up @@ -1127,7 +1084,6 @@ export default optimist( combineReducers( {
currentPost,
isTyping,
blockSelection,
provisionalBlockClientId,
blocksMode,
blockListSettings,
isInsertionPointVisible,
Expand Down
12 changes: 0 additions & 12 deletions packages/editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1749,18 +1749,6 @@ export function isPublishingPost( state ) {
return !! stateBeforeRequest && ! isCurrentPostPublished( stateBeforeRequest );
}

/**
* Returns the provisional block client ID, or null if there is no provisional
* block.
*
* @param {Object} state Editor state.
*
* @return {?string} Provisional block client ID, if set.
*/
export function getProvisionalBlockClientId( 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 hesitated to keep this selector fo BC but the probability for it to be used outside Gutenberg is 0 IMO

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we could keep the function, and have it return null (the default reducer state) so it would at least not throw an error and be as close as possible to a real value with the state no longer existing.

But I'd probably agree it's unlikely that this is used.

return state.provisionalBlockClientId;
}

/**
* Returns whether the permalink is editable or not.
*
Expand Down
46 changes: 1 addition & 45 deletions packages/editor/src/store/test/effects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,61 +21,17 @@ import {
mergeBlocks,
replaceBlocks,
selectBlock,
removeBlock,
createErrorNotice,
setTemplateValidity,
editPost,
} from '../actions';
import effects, {
removeProvisionalBlock,
} from '../effects';
import effects from '../effects';
import * as selectors from '../selectors';
import reducer from '../reducer';

describe( 'effects', () => {
const defaultBlockSettings = { save: () => 'Saved', category: 'common', title: 'block title' };

describe( 'removeProvisionalBlock()', () => {
const store = { getState: () => {} };

beforeAll( () => {
selectors.getProvisionalBlockClientId = jest.spyOn( selectors, 'getProvisionalBlockClientId' );
selectors.isBlockSelected = jest.spyOn( selectors, 'isBlockSelected' );
} );

beforeEach( () => {
selectors.getProvisionalBlockClientId.mockReset();
selectors.isBlockSelected.mockReset();
} );

afterAll( () => {
selectors.getProvisionalBlockClientId.mockRestore();
selectors.isBlockSelected.mockRestore();
} );

it( 'should return nothing if there is no provisional block', () => {
const action = removeProvisionalBlock( {}, store );

expect( action ).toBeUndefined();
} );

it( 'should return nothing if there is a provisional block and it is selected', () => {
selectors.getProvisionalBlockClientId.mockReturnValue( 'chicken' );
selectors.isBlockSelected.mockImplementation( ( state, clientId ) => clientId === 'chicken' );
const action = removeProvisionalBlock( {}, store );

expect( action ).toBeUndefined();
} );

it( 'should return remove action for provisional block', () => {
selectors.getProvisionalBlockClientId.mockReturnValue( 'chicken' );
selectors.isBlockSelected.mockImplementation( ( state, clientId ) => clientId === 'ribs' );
const action = removeProvisionalBlock( {}, store );

expect( action ).toEqual( removeBlock( 'chicken', false ) );
} );
} );

describe( '.MERGE_BLOCKS', () => {
const handler = effects.MERGE_BLOCKS;
const defaultGetBlock = selectors.getBlock;
Expand Down
95 changes: 0 additions & 95 deletions packages/editor/src/store/test/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import {
preferences,
saving,
notices,
provisionalBlockClientId,
blocksMode,
isInsertionPointVisible,
reusableBlocks,
Expand Down Expand Up @@ -1790,100 +1789,6 @@ describe( 'state', () => {
} );
} );

describe( 'provisionalBlockClientId()', () => {
const PROVISIONAL_UPDATE_ACTION_TYPES = [
'UPDATE_BLOCK_ATTRIBUTES',
'UPDATE_BLOCK',
'CONVERT_BLOCK_TO_REUSABLE',
];

const PROVISIONAL_REPLACE_ACTION_TYPES = [
'REPLACE_BLOCKS',
'REMOVE_BLOCKS',
];

it( 'returns null by default', () => {
const state = provisionalBlockClientId( undefined, {} );

expect( state ).toBe( null );
} );

it( 'returns the clientId of the first inserted provisional block', () => {
const state = provisionalBlockClientId( null, {
type: 'INSERT_BLOCKS',
isProvisional: true,
blocks: [
{ clientId: 'chicken' },
],
} );

expect( state ).toBe( 'chicken' );
} );

it( 'does not return clientId of block if not provisional', () => {
const state = provisionalBlockClientId( null, {
type: 'INSERT_BLOCKS',
blocks: [
{ clientId: 'chicken' },
],
} );

expect( state ).toBe( null );
} );

it( 'returns null on block reset', () => {
const state = provisionalBlockClientId( 'chicken', {
type: 'RESET_BLOCKS',
} );

expect( state ).toBe( null );
} );

it( 'returns null on block update', () => {
PROVISIONAL_UPDATE_ACTION_TYPES.forEach( ( type ) => {
const state = provisionalBlockClientId( 'chicken', {
type,
clientId: 'chicken',
} );

expect( state ).toBe( null );
} );
} );

it( 'does not return null if update occurs to non-provisional block', () => {
PROVISIONAL_UPDATE_ACTION_TYPES.forEach( ( type ) => {
const state = provisionalBlockClientId( 'chicken', {
type,
clientId: 'ribs',
} );

expect( state ).toBe( 'chicken' );
} );
} );

it( 'returns null if replacement of provisional block', () => {
PROVISIONAL_REPLACE_ACTION_TYPES.forEach( ( type ) => {
const state = provisionalBlockClientId( 'chicken', {
type,
clientIds: [ 'chicken' ],
} );

expect( state ).toBe( null );
} );
} );

it( 'does not return null if replacement of non-provisional block', () => {
PROVISIONAL_REPLACE_ACTION_TYPES.forEach( ( type ) => {
const state = provisionalBlockClientId( 'chicken', {
type,
clientIds: [ 'ribs' ],
} );

expect( state ).toBe( 'chicken' );
} );
} );
} );

describe( 'blocksMode', () => {
it( 'should set mode to html if not set', () => {
const action = {
Expand Down
19 changes: 0 additions & 19 deletions packages/editor/src/store/test/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ const {
isPublishingPost,
canInsertBlockType,
getInserterItems,
getProvisionalBlockClientId,
isValidTemplate,
getTemplate,
getTemplateLock,
Expand Down Expand Up @@ -3617,24 +3616,6 @@ describe( 'selectors', () => {
} );
} );

describe( 'getProvisionalBlockClientId()', () => {
it( 'should return null if not set', () => {
const provisionalBlockClientId = getProvisionalBlockClientId( {
provisionalBlockClientId: null,
} );

expect( provisionalBlockClientId ).toBe( null );
} );

it( 'should return ClientId of provisional block', () => {
const provisionalBlockClientId = getProvisionalBlockClientId( {
provisionalBlockClientId: 'chicken',
} );

expect( provisionalBlockClientId ).toBe( 'chicken' );
} );
} );

describe( 'isValidTemplate', () => {
it( 'should return true if template is valid', () => {
const state = {
Expand Down
Loading