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

Lighter block DOM: contextual toolbar in popover #18779

Merged
merged 10 commits into from Dec 23, 2019
Merged

Conversation

@ellatrix
Copy link
Member

ellatrix commented Nov 27, 2019

Description

Edit: I decided to break this down in more steps, so this PR now only focuses on the contextual toolbar.

Fixes #7962.

This PR tries to use the Popover component for the contextual toolbar and breadcrumb (navigation mode).

Why?

  • It will be possible to stick the toolbar for multiple block selections. See #7962.
  • This is a step down the path of simplifying the block DOM, and eventually remove all wrapper divs. Why do we want that?
    • Theme authors would have it easier to style block in the editor as they'll look much closer (I'm hoping 100%) look as on the front-end.
    • This could make alignment much easier (we can get rid of the data-align attribute).
    • It allows us to build blocks with inner block that have a semantic relationship. See #18768.
    • In the future, we could allow a block type to set a wrapper tagName. For example, the image block could set it to figure, and a table block to table.
  • It will be possible to let block controls overflow the block easily. Imagine a narrow column. The toolbar could overflow towards the centre. See #18667.
  • Maybe more? Cc @jasmussen.

We already use Popover for a lot of UI (dropdown, inline toolbar, tooltips...), and the component continues to get better. For this PR, we'll need to make some adjustments to it so that the controls position correctly.

How has this been tested?

Screenshots

Types of changes

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Nov 27, 2019

I love the purpose of this PR. Outside of making the DOM simpler (which vastly simplifies block editor block styling), it solves numerous issues where the toolbar is cropped in nested contexts, or just weirdly offset to the right. This is not counting all the other benefits you mention.

If we could get the PR to a state where basically everything looks the same in the branch as it does in master, just with cleaner DOM, this would be great to ship.

Future improvements could inform efforts in #18667.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 27, 2019

Right, I'll work on making everything look the same.

  • I need to adjust Popover with some more positioning options.
  • Make sure Popover repositions smoothly on scroll. Fixed
  • Test on mobile and different screen sizes.
  • Use Popover for the insertion points and buttons.
@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Nov 27, 2019

It does not necessarily need to be 100% accurate, and please let me know if you'd like me to do some of the heavy CSS lifting.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 27, 2019

@jasmussen Thanks! I'm pretty sure I'll need your help to clean up the CSS. I'll let you know when it's otherwise done.

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Nov 27, 2019

Nice! One possible benefit of this change is that it could make #7962 easier to fix.

Some things I've noticed on this branch (I know it's WIP; just posting to point them out):

  • Drag-and-drop is broken.
  • When scrolling, the block toolbar lags behind slightly. I'm not sure if there's any way to fix this without reducing performance, and it isn't really that important, but I do kind of miss the buttery smooth movement of the current toolbar.
  • Similar to the previous issue: when changing selection from a block that has a short block inspector to another block that has an inspector tall enough to create a scroll bar, the block toolbar appears and then a moment later shifts to the left slightly due to the scroll bar shrinking the editor canvas slightly.
@ellatrix ellatrix force-pushed the try/block-controls-popover branch from b83464d to 3c9b4e3 Nov 27, 2019
@ellatrix ellatrix changed the base branch from try/popover-parameters to master Nov 27, 2019
@ellatrix ellatrix force-pushed the try/block-controls-popover branch 3 times, most recently from e522411 to ef48862 Nov 27, 2019
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 27, 2019

I fixed drag and drop of blocks.
The scrolling issues can be fixed.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Nov 28, 2019

What a journey!

First:

The only thing I'm not sure if I can get to work is drag and drop, because the drag handle is in a popover. If it were the block itself it would be easier.

... and then shortly after:

I fixed drag and drop of blocks.

You should get a medal. 🥇

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 28, 2019

@jasmussen Turned out one line was blocking drag 😅

@ellatrix ellatrix force-pushed the try/block-controls-popover branch from cb07089 to a2081c9 Dec 20, 2019
@ellatrix ellatrix requested review from youknowriad and aduth Dec 20, 2019
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 20, 2019

I think this PR is ready for another review. I think we're now very, very close. 😄

@ellatrix ellatrix mentioned this pull request Dec 20, 2019
6 of 6 tasks complete
Copy link
Contributor

youknowriad left a comment

Let's try it. I'm still not very confident with the SlotFill changes. I'd rather find a way to avoid these.

@ellatrix ellatrix force-pushed the try/block-controls-popover branch from a2081c9 to 463a981 Dec 23, 2019
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 23, 2019

There's a small remaining glitch with the toolbar that only happens in Chrome in some conditions. It happens when the content area is an odd number of pixels and as a result, in Chrome only, the reported block position and actual position have a difference of half a pixel, causing the toolbar position to be off by half a pixel.

@ellatrix ellatrix force-pushed the try/block-controls-popover branch from a3d4914 to 17c8ead Dec 23, 2019
@ellatrix ellatrix force-pushed the try/block-controls-popover branch from 17c8ead to 2c345a5 Dec 23, 2019
@ellatrix ellatrix force-pushed the try/block-controls-popover branch from b8ebb3d to 3bd6f07 Dec 23, 2019
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Dec 23, 2019

Okay, I took everything for a last round of testing.

I ensured that:

  • The toolbar is rendered and positioned correctly for:
    • All alignments (none, left, right, wide, and full)
    • Multi-selection
    • Nested blocks
    • Different screen sizes
    • RTL
    • Chrome, Safari, Firefox, Edge and IE11
  • It doesn't affect the mobile block toolbar
  • Moving blocks happens smoothly
  • Scrolling is smooth
  • Popovers anchored within the toolbar are positioned correctly
  • Other popovers have no regressions
  • You can drag and drop a block
  • You can tab in and out of the toolbar
  • Alt+F10 works

If I missed anything, I’ll fix it in a follow-up PR.

To recap, this PR puts the block controls (contextual toolbar and movers) in a popover.

  • It fixes sticky positioning for multiple selected blocks.
  • It reduces the amount of CSS needed to position the toolbar.
  • It allows us to position the toolbar better for narrow blocks. I have not done this yet as the movers still visually appear at the side of the block.
  • It’s a big step towards lightening the block DOM, which eventually will help theme authors, remove our alignment hacks, and could allow us to nest blocks that need a semantic relationship.

This PR introduces some unstable props on Popover, allowing us to figure out how to best stabilise this as the use of Popover grows. This PR has already brought many fixes to Popover benefitting all uses elsewhere.

@ellatrix ellatrix force-pushed the try/block-controls-popover branch from f6d700a to 9830ff9 Dec 23, 2019
@ellatrix ellatrix merged commit c73b6b7 into master Dec 23, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@ellatrix ellatrix deleted the try/block-controls-popover branch Dec 23, 2019
@chrisvanpatten

This comment has been minimized.

Copy link
Member

chrisvanpatten commented Dec 24, 2019

Congrats on getting this merged in @ellatrix — super excited to see what kinds of improvements this unlocks! 👏

@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.