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

Move the post title selection state to the store and update PostTitle #16704

Merged
merged 11 commits into from
Jul 25, 2019
37 changes: 3 additions & 34 deletions packages/edit-post/src/components/visual-editor/index.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,33 +15,6 @@ import { ReadableContentView } from '@wordpress/components';
import styles from './style.scss';

class VisualEditor extends Component {
constructor() {
super( ...arguments );

this.onPostTitleSelect = this.onPostTitleSelect.bind( this );
this.onPostTitleUnselect = this.onPostTitleUnselect.bind( this );

this.state = {
isPostTitleSelected: false,
};
}

static getDerivedStateFromProps( props ) {
if ( props.isAnyBlockSelected ) {
return { isPostTitleSelected: false };
}
return null;
}

onPostTitleSelect() {
this.setState( { isPostTitleSelected: true } );
this.props.clearSelectedBlock();
}

onPostTitleUnselect() {
this.setState( { isPostTitleSelected: false } );
}

renderHeader() {
const {
editTitle,
Expand All @@ -55,9 +28,6 @@ class VisualEditor extends Component {
innerRef={ setTitleRef }
title={ title }
onUpdate={ editTitle }
onSelect={ this.onPostTitleSelect }
onUnselect={ this.onPostTitleUnselect }
isSelected={ this.state.isPostTitleSelected }
placeholder={ __( 'Add title' ) }
borderStyle={
this.props.isFullyBordered ?
Expand Down Expand Up @@ -93,7 +63,7 @@ class VisualEditor extends Component {
isFullyBordered={ isFullyBordered }
rootViewHeight={ rootViewHeight }
safeAreaBottomInset={ safeAreaBottomInset }
isPostTitleSelected={ this.state.isPostTitleSelected }
isPostTitleSelected={ this.props.isPostTitleSelected }
Copy link
Contributor

@mchowning mchowning Jul 22, 2019

Choose a reason for hiding this comment

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

Just curious, is there a reason to use the isPostTitleSelected selector here in visual-editor and pass the resulting value down to the BlockList as a prop instead of just using the isPostTitleSelected selector directly inside of BlockList itself?

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 simply wanted to minimize the changes to the native part here. #16677 will be responsible of refactoring the VisualEditor and moving the block picker out of BlockList into an Inserter component.

onBlockTypeSelected={ this.onPostTitleUnselect }
/>
</BlockEditorProvider>
Expand All @@ -106,14 +76,13 @@ export default compose( [
const {
getEditorBlocks,
getEditedPostAttribute,
isPostTitleSelected,
} = select( 'core/editor' );

const { getSelectedBlockClientId } = select( 'core/block-editor' );

return {
blocks: getEditorBlocks(),
title: getEditedPostAttribute( 'title' ),
isAnyBlockSelected: !! getSelectedBlockClientId(),
isPostTitleSelected: isPostTitleSelected(),
};
} ),
withDispatch( ( dispatch ) => {
Expand Down
64 changes: 45 additions & 19 deletions packages/editor/src/components/post-title/index.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { isEmpty } from 'lodash';
import { Component } from '@wordpress/element';
import { RichText } from '@wordpress/rich-text';
import { decodeEntities } from '@wordpress/html-entities';
import { withDispatch } from '@wordpress/data';
import { withDispatch, withSelect } from '@wordpress/data';
import { withFocusOutside } from '@wordpress/components';
import { withInstanceId, compose } from '@wordpress/compose';
import { __, sprintf } from '@wordpress/i18n';
Expand All @@ -22,6 +22,13 @@ import { pasteHandler } from '@wordpress/blocks';
import styles from './style.scss';

class PostTitle extends Component {
componentDidUpdate( prevProps ) {
// Unselect if any other block is selected
if ( this.props.isSelected && ! prevProps.isAnyBlockSelected && this.props.isAnyBlockSelected ) {
this.props.onUnselect();
Copy link
Contributor

@mchowning mchowning Jul 24, 2019

Choose a reason for hiding this comment

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

This works well! Only concern I have is that it doesn't feel right to me that we have a component that is essentially checking the state of the store (isPostTitleSelected and isAnyBlockSelected) to determine whether/how to make a further change to the store (updating isPostTitleSelected). To me that sounds like the kind of job that a reducer is for—take the previous store state (isPostTitleSelected) and an action (a block selection action) and update the store.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem here is that isAnyBlockSelected comes from the core/block-editor store and we're updating the core/editor store here so a reducer would not work. I don't believe we should use controls or resolvers either as that would introduce a dependency between the stores.

I guess the actual fix would be to make sure that whenever a block is focused, a blur event is triggered on the post title, listen to that event and update the stateS accordingly

Copy link
Contributor

Choose a reason for hiding this comment

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

I keep forgetting about the multiple stores. I'm fine with this approach in light of that.

}
}

componentDidMount() {
if ( this.props.innerRef ) {
this.props.innerRef( this );
Expand Down Expand Up @@ -91,27 +98,46 @@ class PostTitle extends Component {
}
}

const applyWithDispatch = withDispatch( ( dispatch ) => {
const {
undo,
redo,
} = dispatch( 'core/editor' );
export default compose(
withSelect( ( select ) => {
const {
isPostTitleSelected,
} = select( 'core/editor' );

const {
insertDefaultBlock,
} = dispatch( 'core/block-editor' );
const { getSelectedBlockClientId } = select( 'core/block-editor' );

return {
onEnterPress() {
insertDefaultBlock( undefined, undefined, 0 );
},
onUndo: undo,
onRedo: redo,
};
} );
return {
isAnyBlockSelected: !! getSelectedBlockClientId(),
isSelected: isPostTitleSelected(),
};
} ),
withDispatch( ( dispatch ) => {
const {
undo,
redo,
togglePostTitleSelection,
} = dispatch( 'core/editor' );

export default compose(
applyWithDispatch,
const {
clearSelectedBlock,
insertDefaultBlock,
} = dispatch( 'core/block-editor' );

return {
onEnterPress() {
insertDefaultBlock( undefined, undefined, 0 );
},
onUndo: undo,
onRedo: redo,
onSelect() {
togglePostTitleSelection( true );
clearSelectedBlock();
Copy link
Contributor

Choose a reason for hiding this comment

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

Asking more for my understanding: is it worth considering handling the clearSelectedBlock call inside a reducer for the TOGGLE_POST_TITLE_SELECTION action anytime it is set to true? My thinking is just that we would always want to clear any other selected block anytime we set the post title to be selected, and handling it in a reducer would avoid us needing to remember to always dispatch these two actions together. I do not feel strongly about this, but I would be interested in hearing any reasons for or against this in your mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A reducer should only ever update the state so I don't think it would be appropriate. An action could be calling another action but I think it's more explicit to have it here, especially since it's a different store!

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point about the reducer just updating the store, and I totally overlooked the different store. Thanks! 👍

},
onUnselect() {
togglePostTitleSelection( false );
},
};
} ),
withInstanceId,
withFocusOutside
)( PostTitle );
16 changes: 16 additions & 0 deletions packages/editor/src/store/actions.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@

export * from './actions.js';

/**
* Returns an action object that enables or disables post title selection.
*
* @param {boolean} [isSelected=true] Whether post title is currently selected.

* @return {Object} Action object.
*/
export function togglePostTitleSelection( isSelected = true ) {
return {
type: 'TOGGLE_POST_TITLE_SELECTION',
isSelected,
};
}
64 changes: 64 additions & 0 deletions packages/editor/src/store/reducer.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/**
* External dependencies
*/
import optimist from 'redux-optimist';

/**
* WordPress dependencies
*/
import { combineReducers } from '@wordpress/data';

/**
* Internal dependencies
*/
import {
editor,
initialEdits,
currentPost,
preferences,
saving,
postLock,
reusableBlocks,
template,
previewLink,
postSavingLock,
isReady,
editorSettings,
} from './reducer.js';

export * from './reducer.js';

/**
* Reducer returning the post title state.
*
* @param {PostTitleState} state Current state.
* @param {Object} action Dispatched action.
*
* @return {Object} Updated state.
*/
export const postTitle = combineReducers( {
isSelected( state = false, action ) {
switch ( action.type ) {
case 'TOGGLE_POST_TITLE_SELECTION':
return action.isSelected;
}

return state;
},
} );

export default optimist( combineReducers( {
editor,
initialEdits,
currentPost,
preferences,
saving,
postLock,
reusableBlocks,
template,
previewLink,
postSavingLock,
isReady,
editorSettings,
postTitle,
} ) );
13 changes: 13 additions & 0 deletions packages/editor/src/store/selectors.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@

export * from './selectors.js';

/**
* Returns true if post title is selected.
*
* @param {Object} state Global application state.
*
* @return {boolean} Whether current post title is selected.
*/
export function isPostTitleSelected( state ) {
return state.postTitle.isSelected;
}
17 changes: 17 additions & 0 deletions packages/editor/src/store/test/actions.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@

/**
* Internal dependencies
*/
import { togglePostTitleSelection } from '../actions';

describe( 'Editor actions', () => {
describe( 'togglePostTitleSelection', () => {
it( 'should return the TOGGLE_POST_TITLE_SELECTION action', () => {
const result = togglePostTitleSelection( true );
expect( result ).toEqual( {
type: 'TOGGLE_POST_TITLE_SELECTION',
isSelected: true,
} );
} );
} );
} );
34 changes: 34 additions & 0 deletions packages/editor/src/store/test/reducer.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* Internal dependencies
*/
import {
postTitle,
} from '../reducer';

describe( 'state native', () => {
describe( 'postTitle', () => {
describe( 'isSelected()', () => {
it( 'should not be selected by default', () => {
expect( postTitle( undefined, {} ).isSelected ).toBe( false );
} );

it( 'should return false if not selecting the post title', () => {
const action = {
type: 'TOGGLE_POST_TITLE_SELECTION',
isSelected: false,
};

expect( postTitle( { isSelected: true }, action ).isSelected ).toBe( false );
} );

it( 'should return true if selecting the post title', () => {
const action = {
type: 'TOGGLE_POST_TITLE_SELECTION',
isSelected: true,
};

expect( postTitle( { isSelected: false }, action ).isSelected ).toBe( true );
} );
} );
} );
} );
28 changes: 28 additions & 0 deletions packages/editor/src/store/test/selectors.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* Internal dependencies
*/
import { isPostTitleSelected } from '../selectors';

describe( 'selectors native', () => {
describe( 'isPostTitleSelected', () => {
it( 'should return true if the post title is selected', () => {
const state = {
postTitle: {
isSelected: true,
},
};

expect( isPostTitleSelected( state ) ).toBe( true );
} );

it( 'should return false if the post title is not selected', () => {
const state = {
postTitle: {
isSelected: false,
},
};

expect( isPostTitleSelected( state ) ).toBe( false );
} );
} );
} );