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

Fix sibling inserter hit area #9229

Merged
merged 1 commit into from
Aug 23, 2018
Merged

Conversation

jasmussen
Copy link
Contributor

This fixes #8881.

The sibling inserter hit area was not centered perfectly between two blocks. This PR fixes that.

Props to @chrisvanpatten for this fix.

The issue, as described in #8881, is that the sibling inserter covered actual text of text blocks. Specifically, it has a large empty hit area whose sole purpose is to show you the sibling inserter button when you hover that area. However this was misaligned, and covered text below.

This fixes that by aligning it correctly. Here's a GIF with debug colors. The orange shows the sibling inserter hit area, the red shows the text input fields:

now

Noting to myself that if we move further with #8350, we need to account for that with the sibling inserter positioning and height. I have some ideas, including simply having a smaller minimum height of the hoverable area, but this is something to think about. The code in question is this:

screen shot 2018-08-22 at 09 44 10

This fixes #8881.

The sibling inserter hit area was not centered perfectly between two blocks. This PR fixes that.

Props to @chrisvanpatten for this fix.
@jasmussen jasmussen added the [Type] Bug An existing feature does not function as intended label Aug 22, 2018
@jasmussen jasmussen added this to the 3.7 milestone Aug 22, 2018
@jasmussen jasmussen self-assigned this Aug 22, 2018
@jasmussen jasmussen requested a review from a team August 22, 2018 07:48
@chrisvanpatten
Copy link
Contributor

Is there any reason not to just turn this off on selected blocks entirely since we don't show an inserter there anyway?

.editor-block-list__block.is-selected > .editor-block-list__insertion-point {
  display: none;
}

The only case this could have an impact in the future is if we decide to show the insertion point on selected blocks without toolbars. It'd have to be rewritten with a selector like .editor-block-list__block.is-selected:not(.has-toolbar). But that's a concern for the future!

@jasmussen
Copy link
Contributor Author

Is there any reason not to just turn this off on selected blocks entirely since we don't show an inserter there anyway?

Yes, you have to be able to tab into it. It's not super ideal, and something that can be looked at separately as a future thing. But it's a rather biggish refactor to change this with tab order and such.

@chrisvanpatten
Copy link
Contributor

TIL the inserter is available via tab even when hidden from the mouse!

@ZebulanStanphill
Copy link
Member

Speaking of which, I really think the sibling inserter belongs below blocks, rather than above. It makes so much more sense, in my opinion.

Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

Looks good. Not noticing any regressions 🙂👍

@jasmussen jasmussen merged commit e959064 into master Aug 23, 2018
@jasmussen jasmussen deleted the fix/sibling-inserter-hit-area branch August 23, 2018 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inserter: sibling insertion point at top of selected block covers up part of block content
4 participants