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

[RNMobile] When inserting block from title replace block if appropriate #16574

Merged
merged 1 commit into from Jul 19, 2019

Conversation

@mchowning
Copy link
Contributor

commented Jul 13, 2019

Related gutenberg-mobile PR

Description

Fixes an issue where inserting a block from the post title would not replace an empty default block if that block was the first block in the post. This would allow the user to, for example, repeatedly insert consecutive empty paragraph blocks at the beginning of the post without the blocks ever being merged/replaced.

Before After
before_double-insert mp4 after_double-insert mp4

Test scenarios

Test the fix

  1. Open the sample app
  2. Select the post title
  3. Add a paragraph block
  4. Observe that there is a single empty paragraph block at the top of the post, which is now selected
  5. Select the post title again, leaving the just added paragraph at the top of the post empty
  6. Add a second paragraph block
  7. Observe that (a) there is only a single empty paragraph block at the top of the post; and (b) that single empty paragraph block at the top of the post is selected

Regression Checks

  • Adding any block from the post title should not replace the block at the top of the post if the block at the top of the post is not an empty paragraph block
  • Adding any block when an empty paragraph block is selected should replace that empty paragraph block with the newly added block
  • Adding any block when a non-empty paragraph block is selected should insert the newly added block below the selected non-empty paragraph block

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.

@mchowning mchowning self-assigned this Jul 13, 2019

// so replaceBlock does not select the new block and we need to manually select the new block
const { clientId: firstBlockId } = this.props.firstBlock;
this.props.replaceBlock( firstBlockId, newBlock );
this.props.selectBlock( newBlock.clientId );

This comment has been minimized.

Copy link
@mchowning

mchowning Jul 13, 2019

Author Contributor

Some additional context for this line:

If replacing the first block of the post when the title selected, it is also necessary to select the new block at the beginning of the post. This is because the block selection reducer checks and only updates the block selection if the action's clientId matches the clientId from that state's start property. When the post's title is selected, the state's start property is an empty object, so the undefined clientId does not match the action's clientId and the block selection is not updated. When a block is selected (such as if you're replacing an empty paragraph block) then the clientId's do match and the selection state is updated, resulting in the new block being selected. I feel that just sending of the SELECT_BLOCK action works well for this rather-edge case, and it avoids the complexity/risk inherent in updating the blockSelection reducer we share with web. I'm certainly open to suggestions for better ways to handle this though.

@mchowning mchowning changed the title [RNMobile] When inserting block from title replace block when appropriate [RNMobile] When inserting block from title replace block if appropriate Jul 13, 2019

@mchowning mchowning requested review from Tug and marecar3 Jul 13, 2019


return {
blockClientIds: getBlockOrder( rootClientId ),

This comment has been minimized.

Copy link
@marecar3

marecar3 Jul 18, 2019

Contributor

Hey @mchowning nice work here :)
Just curious about this line of the code. Why we don't need to set blockClientIds anymore with getBlockOrder( rootClientId ) ?

This comment has been minimized.

Copy link
@mchowning

mchowning Jul 19, 2019

Author Contributor

We definitly still need this! My intention was just to extract the assignment of getBlockOrder( rootClientId ) to a blockClientIds variable on line 294, and then use that variable to return both the blockClientIds (line 297) and the firstBlock (line 302):

		const blockClientIds = getBlockOrder( rootClientId );     // line 294

		return {
			blockClientIds,                                   // line 297
                         [...]
			firstBlock: getBlock( blockClientIds[ 0 ] ),      // line 302
                         [...]
		};

I believe my changes should not alter the value that is being returned as the blockClientIds, so if I am altering that in some way, definitely let me know.

This comment has been minimized.

Copy link
@marecar3

marecar3 Jul 19, 2019

Contributor

Thanks, @mchowning, I definitely missed 294.
It seems good to me.

blockCount: getBlockCount( rootClientId ),
getBlockName,
isBlockSelected,
selectedBlock: getSelectedBlock(),
firstBlock: getBlock( blockClientIds[ 0 ] ),

This comment has been minimized.

Copy link
@marecar3

marecar3 Jul 18, 2019

Contributor

Should we defend ourselves here if blockClientIds() at some scenario can be null?

This comment has been minimized.

Copy link
@mchowning

mchowning Jul 19, 2019

Author Contributor

Currently, we're protected from this because blockClientIds is assigned from getBlockOrder( rootClientId ), which will always return at least an empty array:

export function getBlockOrder( state, rootClientId ) {
	return state.blocks.order[ rootClientId || '' ] || EMPTY_ARRAY;
}

I also confirmed having an undefined firstBlock prop (as a result of assigning [][0]) did not cause any problems.

With that said, I wouldn't be opposed to still adding in the protection since it doesn't hurt and it would protect us against a change to the implementation of getBlockOrder. What do you think @marecar3 ?

As an aside, it's cases like this that make me miss working with typescript (or flow). Felt good knowing that the compiler would catch something like this if it was a problem.

This comment has been minimized.

Copy link
@marecar3

marecar3 Jul 19, 2019

Contributor

Thanks for the details explanation. I think we are fine with the current solution :)

@marecar3
Copy link
Contributor

left a comment

Nice work @mchowning !
LGTM!

@mchowning mchowning merged commit b0884b7 into master Jul 19, 2019

1 of 10 checks passed

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

@mchowning mchowning deleted the rnmobile/replace_block_when_inserting_from_title branch Jul 19, 2019

@youknowriad youknowriad added this to the Gutenberg 6.2 milestone Jul 26, 2019

@mchowning mchowning referenced this pull request Jul 26, 2019
0 of 5 tasks complete
@mchowning

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

Adding a link to some discussion that we might not want this change: #16677 (comment)

mchowning added a commit that referenced this pull request Jul 26, 2019

mchowning added a commit that referenced this pull request Jul 26, 2019

mchowning added a commit that referenced this pull request Jul 26, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.