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

Remove focused block on backspace #666

Merged
merged 1 commit into from
May 5, 2017
Merged

Conversation

aduth
Copy link
Member

@aduth aduth commented May 4, 2017

Related: #130

This pull request seeks to add basic backspace behavior for removing a block. We'll need to expand upon this, especially for blocks using the Editable component, as the block rendering cannot know when a block is empty (it should be up to a block to manage this consideration). As such, the changes here work best with blocks which do not auto-focus an Editable, e.g. the <hr> separator block. Blocks with editable text can be deleted by carefully clicking twice on the outer border of the block.

Testing instructions:

Verify that backspace deletes a block:

  1. Select the separator block
  2. Hit backspace
  3. Note that the block is removed

Verify that backspace within a text region does not delete the block:

  1. Select within a text block
  2. Hit backspace
  3. Note that the block is not removed

@aduth aduth added the [Type] Enhancement A suggestion for improvement. label May 4, 2017
@youknowriad
Copy link
Contributor

This makes me think, maybe we should revert this #530 since or maybe just make it obvious in the block API that it can be removed by backspacing the following text block.

@jasmussen
Copy link
Contributor

I was thinking of this the other day. It seems like there are some fine interactions we need to polish.

#530 made sense, because you don't want to sit in a text block after an image block, backspace, and delete the image. But it still makes sense to let you select the image block, press backspace, and delete it. It also makes sense that backspace inside a paragraph block merges it with the previous block, as works now.

What if, when you are in a paragraph after an image and press backspace, first you select the image, and then if you press backspace again, you then delete it? We might need some changes that Ella proposed in a comment on #669 for this to work.

@aduth
Copy link
Member Author

aduth commented May 5, 2017

What if, when you are in a paragraph after an image and press backspace, first you select the image, and then if you press backspace again, you then delete it?

This makes sense to me.

That's consistent with the changes here too, yea? At least in the sense of assuming once the block has received focus.

@aduth aduth force-pushed the update/backspace-delete-block branch from a20b4e0 to c4cfb73 Compare May 5, 2017 14:13
@aduth
Copy link
Member Author

aduth commented May 5, 2017

I'm going to interpret the reactions as implied approval 😆

@aduth aduth merged commit 2eff0c3 into master May 5, 2017
@aduth aduth deleted the update/backspace-delete-block branch May 5, 2017 14:37
@jasmussen
Copy link
Contributor

I'm going to interpret the reactions as implied approval 😆

Explicit approval!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants