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

Block List: Insert blocks after as default block #7870

Merged
merged 4 commits into from
Jul 12, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 10, 2018

Salvaged from #6452

This pull request seeks to normalize the behavior of BlockListBlock's "Enter" key handling to insert the default block, not explicitly the paragraph block.

Testing instructions:

There should be no change in behavior. Notably, pressing Enter while the Image block placeholder is selected should still insert a paragraph (which is the default block unless otherwise configured).

@aduth aduth added [Type] Bug An existing feature does not function as intended [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... labels Jul 10, 2018
@aduth aduth force-pushed the update/block-list-as-insert-default branch from a06b1ca to 5980067 Compare July 10, 2018 20:33
@@ -657,6 +655,7 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps ) => {
blocks = blocks.map( ( block ) => cloneBlock( block, { layout } ) );
insertBlocks( blocks, index, rootUID );
},
onInsertDefaultBlock: insertDefaultBlock,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should expand onInsertDefaultBlock to correctly pass the rooUID and the index to the action insertDefaultBlock. The action receives attributes, rootUID, and index. We are just passing the index as attributes.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

I noticed two bugs:
Insert 2 image placeholders. Select the first image press enter. The new paragraph is created after the second image. In master, it was created in the middle.
Add a columns block and add an image inside, press enter on the placeholder, the new paragraph is created outside of the columns.

I think both bugs are the result of the problem described in https://github.com/WordPress/gutenberg/pull/7870/files#r201515058.

aduth added 4 commits July 11, 2018 11:23
Avoids issues around focus transitions when moving between code and visual editor†, improves performance.

† Worth noting that these are legitimate issues, but ones which shouldn't be "covered" by a utility, but rather as part of its own suite of tests.
@aduth aduth force-pushed the update/block-list-as-insert-default branch from 5980067 to 8028a7f Compare July 11, 2018 15:37
@aduth
Copy link
Member Author

aduth commented Jul 11, 2018

Nice catch @jorgefilipecosta . I've pushed a few updates which should fix this. I added a new end-to-end test case, though it doesn't cover much beyond testing the Enter/Delete behaviors while the block focus boundary (e.g. image placeholder) is selected, which is certainly better than what we have. Since I was having some issues implementing it with our current getHTMLFromCodeEditor, I also took additional steps to refactor this function to use window.wp.data.select instead.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Things look fine in my tests now 👍

@aduth aduth merged commit 645e110 into master Jul 12, 2018
@aduth aduth deleted the update/block-list-as-insert-default branch July 12, 2018 20:51
aduth added a commit that referenced this pull request Jul 17, 2018
Previously getHTMLFromCodeEditor , renamed in #7870 and not accounted for in a rebase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants