Skip to content
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
4 changes: 2 additions & 2 deletions src/app/AppContainer.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ const mapDispatchToProps = ( dispatch, ownProps ) => {
deleteBlockAction: ( clientId ) => {
dispatch( deleteBlockAction( clientId ) );
},
createBlockAction: ( clientId, block ) => {
dispatch( createBlockAction( clientId, block ) );
createBlockAction: ( clientId, block, clientIdAbove ) => {
dispatch( createBlockAction( clientId, block, clientIdAbove ) );
},
};
};
Expand Down
11 changes: 3 additions & 8 deletions src/block-management/block-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export type BlockListType = {
moveBlockUpAction: string => mixed,
moveBlockDownAction: string => mixed,
deleteBlockAction: string => mixed,
createBlockAction: ( string, BlockType ) => mixed,
createBlockAction: ( string, BlockType, string ) => mixed,
blocks: Array<BlockType>,
aztechtml: string,
refresh: boolean,
Expand Down Expand Up @@ -72,16 +72,11 @@ export default class BlockManager extends React.Component<PropsType, StateType>
this.props.deleteBlockAction( clientId );
break;
case ToolbarButton.PLUS:
// TODO: direct access insertion: it would be nice to pass the dataSourceBlockIndex here,
// so in this way we know the new block should be inserted right after this one
// instead of being appended to the end.
// this.props.createBlockAction( uid, dataSourceBlockIndex );

// TODO: block type picker here instead of hardcoding a core/code block
const newBlock = createBlock( 'core/paragraph', { content: 'new test text for a core/paragraph block' } );
const newBlockWithFocusedState = { ...newBlock, focused: false };
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's not part of the PR's diff but, wdyt about doing a small optimization here by just appending to the object a new field instead of creating a full new object?

We can just do:

newBlock.focused = false;

and use the newBlock instead of construct the newBlockWithFocusedState.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call! decided to address that in 39f6500 in #112 given this part of the code gets moved around in there 👍

this.state.dataSource.push( newBlockWithFocusedState );
this.props.createBlockAction( newBlockWithFocusedState.clientId, newBlockWithFocusedState );
this.state.dataSource.splice( dataSourceBlockIndex + 1, 0, newBlockWithFocusedState );
this.props.createBlockAction( newBlockWithFocusedState.clientId, newBlockWithFocusedState, clientId );
break;
case ToolbarButton.SETTINGS:
// TODO: implement settings
Expand Down
13 changes: 11 additions & 2 deletions src/store/actions/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,20 @@
*/

import ActionTypes from './ActionTypes';
import type { BlockType } from '../';

export type BlockActionType = string => {
type: $Values<typeof ActionTypes.BLOCK>,
clientId: string,
};

export type CreateActionType = ( string, BlockType, string ) => {
type: $Values<typeof ActionTypes.BLOCK>,
clientId: string,
block: BlockType,
clientIdAbove: string,
};

export function updateBlockAttributes( clientId: string, attributes: mixed ) {
return {
type: ActionTypes.BLOCK.UPDATE_ATTRIBUTES,
Expand Down Expand Up @@ -38,8 +46,9 @@ export const deleteBlockAction: BlockActionType = clientId => ( {
clientId,
} );

export const createBlockAction: BlockActionType = (clientId, block) => ( {
export const createBlockAction: CreateActionType = ( clientId, block, clientIdAbove ) => ( {
type: ActionTypes.BLOCK.CREATE,
block: block,
clientId,
block: block,
clientIdAbove,
} );
6 changes: 3 additions & 3 deletions src/store/actions/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ describe( 'Store', () => {
beforeAll( () => {
registerCoreBlocks();
} );

it( 'should create an action to focus a block', () => {
const action = actions.focusBlockAction( '1' );
expect( action.type ).toBeDefined();
Expand Down Expand Up @@ -42,11 +42,11 @@ describe( 'Store', () => {

it( 'should create an action to create a block', () => {
const newBlock = createBlock( 'core/code', { content: 'new test text for a core/code block' } );
const action = actions.createBlockAction( '1', newBlock );
const action = actions.createBlockAction( '1', newBlock, '0' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also test the clientIdAbove while at it, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 8814dc0

expect( action.type ).toEqual( ActionTypes.BLOCK.CREATE );
expect( action.clientId ).toEqual( '1' );
expect( action.block ).toEqual( newBlock );
expect( action.clientIdAbove ).toEqual( '0' );
} );

} );
} );
11 changes: 10 additions & 1 deletion src/store/reducers/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,15 @@ function findBlockIndex( blocks, clientId: string ) {
} );
}

/*
* insert block into blocks[], below / after block having clientIdAbove
*/
function insertBlock( blocks, block, clientIdAbove ) {
// TODO we need to set focused: true and search for the currently focused block and
// set that one to `focused: false`.
blocks.splice( findBlockIndex( blocks, clientIdAbove ) + 1, 0, block );
}

export const reducer = (
state: StateType = { blocks: [], refresh: false },
action: BlockActionType
Expand Down Expand Up @@ -111,7 +120,7 @@ export const reducer = (
case ActionTypes.BLOCK.CREATE: {
// TODO we need to set focused: true and search for the currently focused block and
// set that one to `focused: false`.
blocks.push(action.block);
insertBlock( blocks, action.block, action.clientIdAbove );
return { blocks: blocks, refresh: ! state.refresh };
}
default:
Expand Down