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

Conversation

@Tug
Copy link
Contributor

commented Jul 22, 2019

Description

This PR moves the selection state of the block title to the core/editor store so we can access it from outside the PostTitle component.

This addition allows us to better implement new features such as wordpress-mobile/gutenberg-mobile#1219 which was added for native. In this case, being able to add a new block at the beginning of the block list when the post title is selected. This PR does not intend to add this behavior for the web.

Types of changes

Refactor of the PostTitle component and add new postTitle reducer in core/editor.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

cc @iamthomasbishop @mchowning

@Tug Tug self-assigned this Jul 22, 2019

@Tug Tug added this to the Future milestone Jul 22, 2019

@Tug Tug marked this pull request as ready for review Jul 22, 2019

@Tug Tug requested review from talldan and youknowriad as code owners Jul 22, 2019

@Tug Tug requested review from mchowning and hypest Jul 22, 2019

@mchowning

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

👋 @Tug . This looks nice! For mobile, did you want to also remove the isPostTitleSelected state from visual-editor and migrate block-list to use the new post-title selected state you're adding to the store?

@Tug

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

@mchowning right, added to the PR 👍

@@ -93,7 +63,7 @@ class VisualEditor extends Component {
isFullyBordered={ isFullyBordered }
rootViewHeight={ rootViewHeight }
safeAreaBottomInset={ safeAreaBottomInset }
isPostTitleSelected={ this.state.isPostTitleSelected }
isPostTitleSelected={ this.props.isPostTitleSelected }

This comment has been minimized.

Copy link
@mchowning

mchowning Jul 22, 2019

Contributor

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?

This comment has been minimized.

Copy link
@Tug

Tug Jul 22, 2019

Author Contributor

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.

@@ -186,6 +173,13 @@ const applyWithDispatch = withDispatch( ( dispatch ) => {
onUndo: undo,
onRedo: redo,
clearSelectedBlock,

This comment has been minimized.

Copy link
@mchowning

mchowning Jul 23, 2019

Contributor

Do we still need to include clearSelectedBlock as a prop here on 175 now that it is only being used by onSelect on line 178?

This comment has been minimized.

Copy link
@Tug

Tug Jul 23, 2019

Author Contributor

Indeed it's not needed anymore 👍

togglePostTitleSelection( true );
clearSelectedBlock();
},
onUnselect() {

This comment has been minimized.

Copy link
@mchowning

mchowning Jul 23, 2019

Contributor

I didn't know es6 allowed us to declare functions this way. Thanks for the TIL! 😎

@@ -32,10 +32,6 @@ class PostTitle extends Component {
this.props.onUnselect();
}

focus() {

This comment has been minimized.

Copy link
@mchowning

mchowning Jul 23, 2019

Contributor

Currently, we pass a ref to this component up to the editor and use this focus method in the Editor:

this.subscriptionParentSetFocusOnTitle = subscribeSetFocusOnTitle( () => {
	if ( this.postTitleRef ) {
		this.postTitleRef.focus();
	}
} );

I'm very excited that your change will allow us to stop passing the ref up there and make that call (the Editor could just directly emit your new TOGGLE_POST_TITLE_SELECTION action now without needing the ref). Until we make that change though we need to keep this focus method because the app will try to call that method to focus the title anytime an empty new post is opened.

This comment has been minimized.

Copy link
@Tug

Tug Jul 23, 2019

Author Contributor

Indeed, good catch 👍

onRedo: redo,
onSelect() {
togglePostTitleSelection( true );
clearSelectedBlock();

This comment has been minimized.

Copy link
@mchowning

mchowning Jul 23, 2019

Contributor

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.

This comment has been minimized.

Copy link
@Tug

Tug Jul 23, 2019

Author Contributor

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!

This comment has been minimized.

Copy link
@mchowning

mchowning Jul 23, 2019

Contributor

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

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

Are these selectors/actions./reducer mobile specific? Should we create an "augmented" version of the store for mobile in that case?

Midterm, we'll want to replace the PostTitle component by a PostTitle block which would make this unnecessary because block's selection is in state.

@Tug

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

Are these selectors/actions./reducer mobile specific? Should we create an "augmented" version of the store for mobile in that case?

There are mobile specific at this point and yes we could create an augmented version of the store for mobile in that case. Though I updated it for the web as well because I believed this feature would be useful to have, so please see it as a proposal :)

Midterm, we'll want to replace the PostTitle component by a PostTitle block which would make this unnecessary because block's selection is in state.

Very interesting, do you have a link to a discussion I could look at? I wonder how it would work given that blocks are reusable by nature 🤔

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

do you have a link to a discussion I could look at? I wonder how it would work given that blocks are reusable by nature

There's some initial discussions on this issue #11553 but also you can take a look at this very hacky/poc branch #16565

This branch is not meant to land as is, it's a way to try things. It contains a post title block and all this is related to Full Site Editing explorations.

@Tug

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

@youknowriad Regardless of the fact that PostTitle will be a block midterm, do you think this is an improvement over the current situation? If not could you confirm that you would rather see those changes added to a native version of the editor store?

@mchowning
Copy link
Contributor

left a comment

Thanks @Tug ! LGTM! 👍

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

Since we're actively working on the post title block, I think I'd rather not introduce these selectors especially with the backward compatibility concerns in mind... at the moment. Making them mobile specific make sense. We should also start thinking about how Full Site Editing work will fit with mobile.

@hypest

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

cc @pinarol to keep in the loop since the comments by @youknowriad link this PostTile PR with related FSE efforts.

Tug added some commits Jul 24, 2019

@Tug

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

@mchowning I extracted the selector/action/reducer into its own version of the code/editor store. Would you mind giving this PR another look?

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

Thanks for the update @Tug 👍

@mchowning
Copy link
Contributor

left a comment

In retesting this, I'm noticing a regression that I missed the first time. The post-title is remaining selected after adding a non-text (i.e., image) block. This is the recent PR fixing that issue. I suspect the regression is is due to the removal of the getDerivedStateFromProps method and its update of the isPostTitleSelected state from the visual-editor (which admittedly doesn't really make much sense there anymore now that the visual-editor doesn't own the isPostTitleSelected state).

On a related note, because of the removal of getDerivedStateFromProps, I believe the visual-editor no longer needs the isAnyBlockSelected prop.

@Tug Tug referenced this pull request Jul 24, 2019
0 of 5 tasks complete
@Tug

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

@mchowning thanks for reporting this, it should be fixed now!

componentDidUpdate( prevProps ) {
// Unselect if any other block is selected
if ( this.props.isSelected && ! prevProps.isAnyBlockSelected && this.props.isAnyBlockSelected ) {
this.props.onUnselect();

This comment has been minimized.

Copy link
@mchowning

mchowning Jul 24, 2019

Contributor

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?

This comment has been minimized.

Copy link
@Tug

Tug Jul 25, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@mchowning

mchowning Jul 25, 2019

Contributor

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

@mchowning
Copy link
Contributor

left a comment

LGTM! Thanks @Tug !

@Tug Tug merged commit dd34d32 into master Jul 25, 2019

1 of 2 checks passed

Filter merged Filter merged
Details
Travis CI - Pull Request Build Passed
Details

@Tug Tug deleted the update/editor/post-title-in-store branch Jul 25, 2019

@youknowriad youknowriad modified the milestones: Future, Gutenberg 6.2 Jul 26, 2019

sbardian added a commit to sbardian/gutenberg that referenced this pull request Jul 29, 2019

Move the post title selection state to the store and update PostTitle (
…WordPress#16704)

* Move the post title selection state to the store and update PostTitle

* Fix jsdoc type returned for postTitle reducer

* Update native version of VisualEditor to use the store to know if the post title is selected

* Update doc

* Remove unused clearSelectedBlock props in PostTitle web and bring back focus in native version

* Fork core/editor store for native instead of modifying it

* Move tests to native

* Update docs

* Revert changes to web PostTitle

* Fix post title blur on block selection

* Update the isPostTitleSelected state if any other block is selected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.