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

Jump to bottom

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

Merged
merged 11 commits into from
Jul 25, 2019
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
630d474
Move the post title selection state to the store and update PostTitle
Tug Jul 22, 2019
4c7ca6a
Fix jsdoc type returned for postTitle reducer
Tug Jul 22, 2019
e1cc965
Update native version of VisualEditor to use the store to know if the…
Tug Jul 22, 2019
e787c58
Update doc
Tug Jul 23, 2019
cc71382
Remove unused clearSelectedBlock props in PostTitle web and bring bac…
Tug Jul 23, 2019
6b38663
Fork core/editor store for native instead of modifying it
Tug Jul 24, 2019
1e248f1
Move tests to native
Tug Jul 24, 2019
243cd5d
Update docs
Tug Jul 24, 2019
5424076
Revert changes to web PostTitle
Tug Jul 24, 2019
db39db5
Fix post title blur on block selection
Tug Jul 24, 2019
b58b35a
Update the isPostTitleSelected state if any other block is selected
Tug Jul 24, 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
34 changes: 3 additions & 31 deletions packages/edit-post/src/components/visual-editor/index.native.js
View file
Open in desktop
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 •
edited
Loading

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,6 +76,7 @@ export default compose( [
const {
getEditorBlocks,
getEditedPostAttribute,
isPostTitleSelected,
} = select( 'core/editor' );

const { getSelectedBlockClientId } = select( 'core/block-editor' );
Expand All @@ -114,6 +85,7 @@ export default compose( [
blocks: getEditorBlocks(),
title: getEditedPostAttribute( 'title' ),
isAnyBlockSelected: !! getSelectedBlockClientId(),
isPostTitleSelected: isPostTitleSelected(),
};
} ),
withDispatch( ( dispatch ) => {
Expand Down
6 changes: 3 additions & 3 deletions packages/editor/src/components/post-title/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,14 +97,15 @@ export default compose(
isSelected: isPostTitleSelected(),
};
} ),
withDispatch( ( dispatch, ownProps ) => {
withDispatch( ( dispatch ) => {
const {
undo,
redo,
togglePostTitleSelection,
} = dispatch( 'core/editor' );

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

Expand All @@ -116,11 +117,10 @@ export default compose(
onRedo: redo,
onSelect() {
togglePostTitleSelection( true );
ownProps.onSelect();
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 );
ownProps.onUnselect();
},
};
} ),
Expand Down