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 selecting separator block #14854

Merged
merged 2 commits into from
Apr 10, 2019

Conversation

ashwin-pc
Copy link
Contributor

Description

Stopgap fix to the issue #12080. It's is an alternate solution to the fix mentioned in #13695. It fixes the inability to select the separator by reducing the insertion point elements height so that the area is still large enough to hover over but not large enough to make selecting the separator impossible.

No visual change will be seen in the editor due to this change.

How has this been tested?

  • Passed all unit tests by running npm test
  • Passed all E2E tests by running npm run test-e2e
  • Manually verified on both desktop and mobile chrome and desktop firefox browsers

Screenshots

Before Change:
image

After change:
image

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.

@gziolo gziolo requested review from jasmussen, kjellr and mapk April 7, 2019 09:26
@gziolo gziolo added [Block] Separator Affects the Separator Block [Type] Bug An existing feature does not function as intended General Interface Parts of the UI which don't fall neatly under other labels. Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code labels Apr 7, 2019
@jasmussen
Copy link
Contributor

Thank you for the PR.

This is a really clever workaround and although I was skeptical in the beginning, this seems like an excellent stopgap solution in anticipation of a better fix in #13723.

So here's what happens:

  • There's an element that sits between every block, unclickable.
  • When you hover that element, the plus button, "Sibling Inserter", appears.
  • The only thing this branch does, is make that element smaller in height.

GIF time. Master with vanilla styles:

master, vanilla styles

Master with 2019 styles:

master, 2019 styles

This branch, with vanilla styles:

branch, vanilla styles

This branch, with 2019 styles:

branch, 2019 styles

As you can see, there's a little funkiness with how TwentyNineteen positions the sibling inserter between paragraphs and blocks. But that is an issue present in master also, so not the fault of this PR.

But overall — the hoverable area still feels responsive, so the plus still feels accessible. But now it just covers less of the block, which as a side-effect makes the separator easy to select.

Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

I left one small comment to change a margin on the clickable plus. But with that fixed, I honestly think this is good to go.

It is, as you note, a stop-gap solution. But it does fix the issue, and the cost: making the sibling inserter EVERY so slightly harder to invoke with a mouse, was not at all an issue in my testing. It does span the full width of the main column, so it's still easy to target.

@@ -737,6 +737,7 @@
left: 0;
right: 0;
justify-content: center;
height: $block-padding + 8px;

// Show a clickable plus.
.block-editor-inserter__toggle {
Copy link
Contributor

Choose a reason for hiding this comment

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

Below this line, it says margin-top: -4px. If you change that to -6px, it better vertically centers the plus between the blocks. This was not a regression of this PR, but would be nice to fix while you're in here.

Copy link
Contributor Author

@ashwin-pc ashwin-pc Apr 8, 2019

Choose a reason for hiding this comment

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

@jasmussen yes that sounds good. I had considered this but avoided making this change as i wasn't sure how some of the E2E tests would handle it if the inserter changed its position, especially since this is only a stop gap fix. Would you still recommend it considering that this is an additional thing to revert when the fix does come in? Also would it be a good idea to add a comment there to indicate that this is indeed a stopgap fix? So that when the real fix does come along these changes are reverted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe. Or maybe we grow to like the change enough to keep it, even if we also fix the other issue.

I can go either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I've made the change to centre align it. I went with -8px instead of -6px as it accurately centre aligns it.

@jasmussen jasmussen added this to the 5.5 (Gutenberg) milestone Apr 8, 2019
@gziolo gziolo merged commit 8cbcb8f into WordPress:master Apr 10, 2019
@gziolo
Copy link
Member

gziolo commented Apr 10, 2019

@ashwin-pc - thanks for fixing this very annoying issue 👍

@gziolo gziolo added the [Type] Regression Related to a regression in the latest release label Apr 10, 2019
@ellatrix
Copy link
Member

It might be good to try to add an e2e test for this. I think this issue has come back several times?

@jasmussen
Copy link
Contributor

It might be good to try to add an e2e test for this. I think this issue has come back several times?

Not sure, I don't think anything regressed prior to this. The separator has been hard to select ever since the last changes to the sibling inserter. The actual problem is outlined in #13723, so we need a developer to tackle that.

mchowning pushed a commit to mchowning/gutenberg that referenced this pull request Apr 15, 2019
* fix selecting separator block

* center align the inserter toggle
aduth pushed a commit that referenced this pull request Apr 16, 2019
* fix selecting separator block

* center align the inserter toggle
aduth pushed a commit that referenced this pull request Apr 16, 2019
* fix selecting separator block

* center align the inserter toggle
This was referenced Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Separator Affects the Separator Block General Interface Parts of the UI which don't fall neatly under other labels. Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants