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

Don't display before block inserter when editor is empty #17508

Open
wants to merge 15 commits into
base: master
from

Conversation

@HardeepAsrani
Copy link
Contributor

commented Sep 21, 2019

Description

This makes sure the before-block inserter isn't displayed when the editor has no content or blocks. Fixes #17416

How has this been tested?

  1. When editor is empty without any blocks, only the default inserter is displayed.
  2. If you add a block or any text in the default paragraph block, the editor will appear.

Screenshots

Before:

Before

After:

After

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.
HardeepAsrani added 14 commits Nov 27, 2017
Update
Merge changes
Fix CodeEditor not loading when WordPress is installed in a subfolder…
Update the git
Merge
Rebase
Merge updates
Added `position` prop to DotTip component. Closes #14923
Rebase
Rebasee
Before this, the additional classes applied to column block didn't show
up in the editor.
Rebase
This makes sure the before-block inserter isn't displayed when editor
has no content or blocks. Fixes #17416
@@ -416,12 +416,13 @@ function BlockListBlock( {
isFirstMultiSelected
);
const shouldShowMobileToolbar = ! isNavigationMode && shouldAppearSelected;
const isEditorEmpty = blockCount <= 1 && isEmptyDefaultBlock;

This comment has been minimized.

Copy link
@youknowriad

youknowriad Sep 25, 2019

Contributor

I have been thinking about this and I think the issue might also happen if the first paragraph is empty but you have other blocks after it right?

I mean should the condition be something like this instead:

isFirstBlock && isEmptyDefaultBlock;

This comment has been minimized.

Copy link
@HardeepAsrani

HardeepAsrani Sep 25, 2019

Author Contributor

Yes, and I tested all such cases and the solution works just fine. If there are multiple blocks, even if empty then it’s fine to have the appender.

Should I do with ‘isFirstBlock’?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Sep 25, 2019

Contributor

If there are multiple blocks, even if empty then it’s fine to have the appender.

Why it's fine to have it for a multiple empty blocks but not when we have just one? In both cases, it seems that we can have the double inserter issue? I'll ping some designers to have thoughts here

@jasmussen @mapk

I was also thinking that using isFirstBlock is better in terms of performance than using blockCount because the blocks won't rerender every time a block is added or removed.

This comment has been minimized.

Copy link
@jasmussen

jasmussen Sep 25, 2019

Contributor

I tend to think we need to step back a little bit and think about the sibling inserter in a more holistic way. I do agree with #17416 in general, it is weird with the double inserter. However to me the solution isn't to add more edgecases. To that end I agree with Riad — simply reducing it to isFirstBlock should be good. An empty block is also a block, and one you might insert other blocks before.

A rule of thumb should be that it should be obvious for users why a thing appears, and subtextually, why it doesn't. It doesn't seem obvious why the sibling inserter would appear at the top when only the block isn't empty.

This comment has been minimized.

Copy link
@HardeepAsrani

HardeepAsrani Sep 25, 2019

Author Contributor

I’m not on a laptop else would’ve made taken some screenshots to explain better but here’s my logic:

When you open the editor with the default block, it’s not a Paragraph block button more like an empty block with Inserter on the left.

But when you add another block below, even if empty, then the first block becomes a proper paragraph block without giving you an Inserter to change it to something else.

Hope that makes sense. 😀

This comment has been minimized.

Copy link
@mapk

mapk Sep 27, 2019

Contributor

I get that an empty block is a block, but it doesn't really act like a block. I can't move it, transform it, or see a block toolbar (for good reasons). So if one other thing on an empty block works differently, that feels okay. In this case, I think if it's an empty block, it just doesn't allow the sibling inserter because the left inserter is already there next to it.

The other hand is that all those actions that an empty block is missing are specific to that block, while removing the sibling inserter is not. It's allowing an action to be made in the page... not in the empty block. So now I think I've come full circle. I can see leaving it in, especially if we improve the interaction with it.

This comment has been minimized.

Copy link
@HardeepAsrani

HardeepAsrani Oct 1, 2019

Author Contributor

So what is the conclusion, if any? :)

This comment has been minimized.

Copy link
@enriquesanchez

enriquesanchez Oct 1, 2019

Contributor

@mapk @jasmussen @youknowriad If an empty block is a block, then why can you "Add a block" on itself?

Screen Shot 2019-10-01 at 4 44 56 PM

The "empty block" is really but not really a paragraph block. But if we treat it more like a "block placeholder", then I would argue that it shouldn't have a prepending block sibling inserter, because no block has really been added.

If, on the contrary, we agree that it is in fact a paragraph block, then I think it makes sense to have a prepending sibling inserter.

A little aside, but maybe related: why don't we show an appending sibling inserter for that first empty block?

This comment has been minimized.

Copy link
@enriquesanchez

enriquesanchez Oct 1, 2019

Contributor

Also worth noting that when navigating with keyboard only, focus first goes to the appending sibling inserter and then to the editable area of the "empty block":

2019-10-01 17-16-10 2019-10-01 17_17_37

This comment has been minimized.

Copy link
@jasmussen

jasmussen Oct 2, 2019

Contributor

I'm willing to defer to whom has a strong opinion on this.

I think there's value in code simplicity, especially because many of these things are not yet 100% solidified, so we should not necessarily optimize towards these things staying the same. For example, we know that the Title will become an actual block at some point. We might even make the placeholder an actual block. Does that change the equation?

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