From 3c050519d020bf0ff4710d968dcfacc088904743 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Wed, 8 Aug 2018 10:39:29 +0100 Subject: [PATCH 1/2] Writing Flow: Remove the provisional block behavior --- docs/data/data-core-editor.md | 15 +--- packages/editor/src/store/actions.js | 5 +- packages/editor/src/store/effects.js | 26 ------ packages/editor/src/store/reducer.js | 44 ---------- packages/editor/src/store/selectors.js | 12 --- packages/editor/src/store/test/effects.js | 46 +--------- packages/editor/src/store/test/reducer.js | 95 --------------------- packages/editor/src/store/test/selectors.js | 19 ----- test/e2e/specs/adding-blocks.test.js | 13 --- 9 files changed, 3 insertions(+), 272 deletions(-) diff --git a/docs/data/data-core-editor.md b/docs/data/data-core-editor.md index 4e421f6a51d4f1..54a62a0568a76a 100644 --- a/docs/data/data-core-editor.md +++ b/docs/data/data-core-editor.md @@ -1,6 +1,6 @@ # **core/editor**: The Editor’s Data -## Selectors +## Selectors ### hasEditorUndo @@ -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. diff --git a/packages/editor/src/store/actions.js b/packages/editor/src/store/actions.js index 79390b9f5dca49..6bb65b38fb8309 100644 --- a/packages/editor/src/store/actions.js +++ b/packages/editor/src/store/actions.js @@ -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 ); } /** diff --git a/packages/editor/src/store/effects.js b/packages/editor/src/store/effects.js index 4027947a60c1cf..6b61df24bb734d 100644 --- a/packages/editor/src/store/effects.js +++ b/packages/editor/src/store/effects.js @@ -27,7 +27,6 @@ import { createWarningNotice, insertBlock, selectBlock, - removeBlock, resetBlocks, setTemplateValidity, } from './actions'; @@ -37,9 +36,7 @@ import { getBlockRootClientId, getBlocks, getPreviousBlockClientId, - getProvisionalBlockClientId, getSelectedBlock, - isBlockSelected, getTemplate, getTemplateLock, } from './selectors'; @@ -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 ); @@ -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 ) { diff --git a/packages/editor/src/store/reducer.js b/packages/editor/src/store/reducer.js index 17334e2829a251..09d58a57be956f 100644 --- a/packages/editor/src/store/reducer.js +++ b/packages/editor/src/store/reducer.js @@ -15,7 +15,6 @@ import { omitBy, keys, isEqual, - includes, overSome, get, } from 'lodash'; @@ -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; @@ -1127,7 +1084,6 @@ export default optimist( combineReducers( { currentPost, isTyping, blockSelection, - provisionalBlockClientId, blocksMode, blockListSettings, isInsertionPointVisible, diff --git a/packages/editor/src/store/selectors.js b/packages/editor/src/store/selectors.js index 05ce02ac882347..ab650632c816ac 100644 --- a/packages/editor/src/store/selectors.js +++ b/packages/editor/src/store/selectors.js @@ -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 ) { - return state.provisionalBlockClientId; -} - /** * Returns whether the permalink is editable or not. * diff --git a/packages/editor/src/store/test/effects.js b/packages/editor/src/store/test/effects.js index 4f4566db92c944..11263cab61f3f0 100644 --- a/packages/editor/src/store/test/effects.js +++ b/packages/editor/src/store/test/effects.js @@ -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; diff --git a/packages/editor/src/store/test/reducer.js b/packages/editor/src/store/test/reducer.js index 525100fb88632a..9445b32a11e7ca 100644 --- a/packages/editor/src/store/test/reducer.js +++ b/packages/editor/src/store/test/reducer.js @@ -29,7 +29,6 @@ import { preferences, saving, notices, - provisionalBlockClientId, blocksMode, isInsertionPointVisible, reusableBlocks, @@ -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 = { diff --git a/packages/editor/src/store/test/selectors.js b/packages/editor/src/store/test/selectors.js index 33326dd179ba70..95594cfcba4c33 100644 --- a/packages/editor/src/store/test/selectors.js +++ b/packages/editor/src/store/test/selectors.js @@ -85,7 +85,6 @@ const { isPublishingPost, canInsertBlockType, getInserterItems, - getProvisionalBlockClientId, isValidTemplate, getTemplate, getTemplateLock, @@ -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 = { diff --git a/test/e2e/specs/adding-blocks.test.js b/test/e2e/specs/adding-blocks.test.js index 02a1ac33487856..f836c894187491 100644 --- a/test/e2e/specs/adding-blocks.test.js +++ b/test/e2e/specs/adding-blocks.test.js @@ -2,7 +2,6 @@ * Internal dependencies */ import { - clickBlockAppender, newPost, insertBlock, getEditedPostContent, @@ -18,18 +17,6 @@ describe( 'adding blocks', () => { // Click below editor to focus last field (block appender) await page.click( '.editor-writing-flow__click-redirect' ); expect( await page.$( '[data-type="core/paragraph"]' ) ).not.toBeNull(); - - // Up to return back to title. Assumes that appender results in focus - // to a new block. - // TODO: Backspace should be sufficient to return to title. - await page.keyboard.press( 'ArrowUp' ); - - // Post is empty, the newly created paragraph has been removed on focus - // out because default block is provisional. - expect( await page.$( '[data-type="core/paragraph"]' ) ).toBeNull(); - - // Using the placeholder - await clickBlockAppender(); await page.keyboard.type( 'Paragraph block' ); // Using the slash command From 94d263ac25a002d7f23e7eba964f1fa2db05131f Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Wed, 8 Aug 2018 15:40:07 +0100 Subject: [PATCH 2/2] Prefer unmodified default block terminology as it already exists in the code --- test/e2e/specs/writing-flow.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/specs/writing-flow.test.js b/test/e2e/specs/writing-flow.test.js index ca67310a986ed9..d28b484be9e079 100644 --- a/test/e2e/specs/writing-flow.test.js +++ b/test/e2e/specs/writing-flow.test.js @@ -149,7 +149,7 @@ describe( 'adding blocks', () => { expect( await getEditedPostContent() ).toMatchSnapshot(); // When returning to Visual mode, backspace in selected block should - // reset to the provisional block. + // reset to an unmodified default block. await page.keyboard.press( 'Backspace' ); // Ensure no data-mce-selected. Notably, this can occur when content