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

use same behavior as Gutenberg web for replacing an empty default block #338

Merged
merged 2 commits into from
Dec 11, 2018

Conversation

mzorz
Copy link
Contributor

@mzorz mzorz commented Dec 10, 2018

Comes from #327 (comment)

While PR #327 addresses #315 description, it's been observed that desired behavior is more that of following along Gutenberg web as much as we can. @koke observed the behavior should be closer to Gutenberg's by using isUnmodifiedDefaultBlock (see https://github.com/WordPress/gutenberg/blob/40d1282a992702e1933118ef626f0666692baf2b/packages/editor/src/components/inserter/menu.js#L389)

Please note that this makes the default block (currently, core/paragraph) the only block type eligible for replacement when such a block is both empty and selected, and the user taps on + to bring up the inserter's block picker.

With this PR then, if the user is standing on an empty Heading or Code block, they won't be able to replace such blocks anymore - the user will only be able to replace a block if they're standing on a Paragraph empty block.

I tested this on Gutenberg web and could observe the same.

To test: (paragraph)

  1. load the demo app
  2. tap on the + inserter icon to bring the BlockType picker up
  3. choose Paragraph type (first on the left)
  4. observe a new, empty Paragraph block is appended to the list and the editor scrolls there (the Paragraph editor gets the focus and the block is selected)
  5. leaving the focus/selection there, tap on the + inserter icon again
  6. now select T title block
  7. observe the block is now a Title block type, as opposed to appending a new empty block once again.

To test: (other editables, non-paragraph)

  1. leaving the focus/selection on the T block inserted in step 6 above:
  2. tap on the + inserter icon
  3. tap on any block
  4. observe a new block is appended, instead of being replaced.

Try steps 8-10 again, inserting a new block while standing on a Code block instead of Title (Heading), and observe in both cases the new block is appended (or inserted below the selected block) rather than replace the previously selected block.

@mzorz mzorz added the Blocks label Dec 10, 2018
@mzorz mzorz added this to the Alpha milestone Dec 10, 2018
@mzorz mzorz requested review from koke, Tug and hypest December 10, 2018 12:17
isReplaceable( block: ?BlockType ) {
if ( ! block ) {
return false;
}
return this.isEmptyBlock( block ) && this.isCandidateForReplaceBlock( block );
return this.isEmptyBlock( block ) && isUnmodifiedDefaultBlock( block );
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 you don't need the empty block check, since isUnmodifiedDefaultBlock already checks if the block has the default attributes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 49e8630 - I still decided to keep isReplaceable() as a method because it makes it easier to read and follow the code where the method is called.

Copy link
Contributor

@Tug Tug left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Unrelated to this PR: I noticed the Heading block fomatting doesn't work when starting from an empty block. I'll open an issue

@mzorz mzorz merged commit 15f2460 into master Dec 11, 2018
@mzorz mzorz deleted the fix/use-isUnmodifiedDefaultBlock-for-replace branch December 11, 2018 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants