Refactor dropzone for unmodified default blocks with useBlockDropZone and InsertionPoint
#44647
Conversation
kevin940726
added
[Type] Enhancement
|
Size Change: -167 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
kevin940726
changed the title
useBlockDropZone and InsertionPoint
| @@ -174,12 +237,10 @@ export default function useBlockDropZone( { | |||
| onDragLeave() { | |||
| throttled.cancel(); | |||
| hideInsertionPoint(); | |||
| setTargetBlockIndex( null ); | |||
There was a problem hiding this comment.
I'm actually not sure why we needed to set it to null here as it'll get updated on drag over anyway?
This is making the hook returned by useOnBlockDrop receive the wrong index sometimes.
There was a problem hiding this comment.
Note that there is some flickering in Safari when dragging in between two blocks:
Kapture.2022-10-04.at.12.34.54.mp4
It's because Safari will always return null for event.relatedTarget in the dragleave events. I think there are ways to fix it but probably deserves another PR.
packages/block-editor/src/components/use-block-drop-zone/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/use-block-drop-zone/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-tools/insertion-point.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-tools/insertion-point.js
Outdated
Show resolved
Hide resolved
| exit: { opacity: 0, scaleY: 0.9 }, | ||
| }; | ||
|
|
||
| function BlockPopoverDropZone( { |
There was a problem hiding this comment.
It might be interesting to see whether this component can be used to solve #44064.
I'd imagine it could cover the rootClientId block when a block list is empty.
(Should be a separate PR though)
packages/block-editor/src/components/use-block-drop-zone/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/use-block-drop-zone/index.js
Outdated
Show resolved
Hide resolved
| const { showInsertionPoint, hideInsertionPoint } = | ||
| useDispatch( blockEditorStore ); | ||
|
|
||
| const onBlockDrop = useOnBlockDrop( targetRootClientId, targetBlockIndex ); | ||
| const onBlockDrop = useOnBlockDrop( targetRootClientId, dropTarget.index, { | ||
| operation: dropTarget.operation, |
There was a problem hiding this comment.
Not necessarily something to do, but thinking aloud - I guess the other option (instead of using an index for the drop target), is to use a clientId and then operation is could be 'insert-before', 'insert-after', 'insert-within' or 'replace'. I always found it hard to make a call on this as there were limited use cases at the time.
index was always weird because it was really the index of the gap between a block rather than the block index itself.
There was a problem hiding this comment.
I had this idea too and I totally agree with you. That would probably be worth a bigger refactor of useOnBlockDrop though.
There was a problem hiding this comment.
I guess backwards compatibility will be difficult 🤔
kevin940726
force-pushed
the
add/block-popover-drop-zone
branch
from
20e380e
to
cce8d8f
Compare
kevin940726
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
kevin940726
force-pushed
the
add/block-popover-drop-zone
branch
from
cce8d8f
to
43b266e
Compare
|
Pinging @ellatrix and @youknowriad in case you missed it ;) |
packages/block-editor/src/components/block-tools/insertion-point.js
Outdated
Show resolved
Hide resolved
Screen.Recording.2022-10-06.at.8.16.23.AM.movI'm noticing two issues in the following recording:
|
|
Thanks for trying it out!
Are you testing it in Safari? There's a known bug in Safari that I think we should fix in a follow-up.
That's intentional though. From the suggestion in the original PR, I think the idea is to avoid showing the in-between indicator when there's an empty paragraph block because the outcome will be the same. That's up for debate though. However, it reminds me of the indicator while hovering in the global inserter. I think we should also change that in a follow-up PR (if that's expected). 🤔 |
Yes,
I guess my point is that the outcome shouldn't be the same, I should leave the empty paragraph and insert after it. At least in my testing it felt weird that the "blue area" was visible when I was clearly not on top of it. cc @mtias |
I think it makes sense if you think of the dropzone as a variant of the blue line indicator. It indicates where the item should be placed. It feels a bit weird to me if you can't drop in between empty paragraphs while you can do that for other block types. Maybe we can come up with a better design though, maybe even in a different issue? |
Yeah, that's my feeling too. Anyway, if this is how it already works in trunk, we can track it separately. |
| if ( | ||
| isUnmodifiedDefaultBlock && | ||
| isPointContainedByRect( position, rect ) | ||
| ) { | ||
| distance = 0; | ||
| } |
There was a problem hiding this comment.
There's probably a chance to break the loop early here.
There was a problem hiding this comment.
I had this plan to use binary search to find the index. Better to do them together in another PR I guess ;)
packages/block-editor/src/components/block-tools/insertion-point.js
Outdated
Show resolved
Hide resolved
| const { showInsertionPoint, hideInsertionPoint } = | ||
| useDispatch( blockEditorStore ); | ||
|
|
||
| const onBlockDrop = useOnBlockDrop( targetRootClientId, targetBlockIndex ); | ||
| const onBlockDrop = useOnBlockDrop( targetRootClientId, dropTarget.index, { | ||
| operation: dropTarget.operation, |
There was a problem hiding this comment.
I guess backwards compatibility will be difficult 🤔
| showInsertionPoint( targetRootClientId, targetIndex, { | ||
| operation, | ||
| } ); |
There was a problem hiding this comment.
This is subjective, but is operation: 'insert' | 'replace' is the right terminology for the action of showInsertionPoint?
This action displays the UI, so I wonder if the option should be something like appearance: 'inbetween' | 'overlay-block'. That might also better match the terminology in the InsertionPoint component.
useOnBlockDrop could still keep the insert/replace terms because it does seem like the right thing there.
I guess the only thing then is becomes a bit of an overhead to translate insert to inbetween and replace to overlay-block.
The other thing is that it is called 'insertion' point, which implies 'insert' already. 🤔
I am nitpicking now, so feel free to ignore 😄
There was a problem hiding this comment.
Yeah, makes sense. It's a bit unfortunate that we're probably too late to change the naming of insertion point now 😅 .
| getBoundingClientRect: () => | ||
| ownerDocument | ||
| .getElementById( `block-${ clientId }` ) | ||
| .getBoundingClientRect(), |
There was a problem hiding this comment.
That is nicer, and it also ensures the blocks don't need to be children of the current element, so should make things more stable. Good improvement 👍
|
Please allow me to have a look at this PR today. |
Could you elaborate on this? Why is this needed? |
| const boundingBox = await this.page | ||
| .locator( selector ) | ||
| .boundingBox(); | ||
| dragOver: async ( |
There was a problem hiding this comment.
Why are we using Playwright here instead of Puppeteer if in Puppeteer drag and drop works out of the box?
There was a problem hiding this comment.
Several reasons:
- We're in the middle of the migration from Puppeteer to Playwright. In order to make the migration path smoother, it makes sense for me to add some missing features in
e2e-test-utils-playwright. Once these utils are implemented, I believe it will be easier for contributors to pick up or add more tests. - Puppeteer doesn't fully support drag-and-drop out of the box either. Specifically, if I'm not mistaken, Puppeteer also doesn't support API for dragging files (Add support to drag and drop files puppeteer/puppeteer#7330). It seems like changes need to be made in Chrome Devtools Protocol to support this. If we implement it in Puppeteer, we would still have to implement these utils anyway.
@talldan has opened an issue (microsoft/playwright#16437) in the Playwright repo already, I believe that's a good start. We can iterate and see where it goes from there ;)
|
I like this way more than the previous implementation! 🙂 When I test dragging a block, I get the following React warning:
|
This is a cleaner implementation as we don't have to lock in to the Paragraph block or any other blocks. The dropzone behaves very similarly to the insertion point, I think it makes sense to combine them both and share common logic. This also allows us to avoid binding event listeners for each dropzone, this could be a performance bottleneck if we render tons of dropzones at the same time.
I believe this is an existing bug in the |
kevin940726
force-pushed
the
add/block-popover-drop-zone
branch
from
43b266e
to
9216c4b
Compare
|
Thanks for iterating on this :) |
What?
A follow-up and refactor of #42722, an alternative to #44606.
This refactor basically reverts the implementation in the Paragraph block (#42722) and re-implements the same feature in
useBlockDropZoneand<InsertionPoint>using<BlockPopover>andisUnmodifiedDefaultBlock(as suggested in #42722 (comment)). This refactor allows us to reuse the same drop zone and their event listeners so that we don't have to bind them elsewhere.Why?
The original PR fails some performance metrics when rendering a large list of empty paragraph blocks and it doesn't take
setDefaultBlockinto account. This PR solves both issues.How?
useBlockDropZone.operationtoreplaceand callshowInsertionPointwith it.<BlockPopover>.This PR has a nice side effect that dragging in-between unmodified default blocks will also trigger the drop zone indicator and allow dropping. This is smoother than the previous implementation, where only hovering on the empty paragraph blocks will trigger the drop zone (which is often very small). See it in action in the below screencast.
Testing Instructions
Screenshots or screencast
Kapture.2022-10-04.at.12.05.16.mp4
How to review
useBlockDropZonefirst, and follow the logic there.block-editor's actions (showInsertionPoint) -> reducer.<InsertionPoint>is where the presentational component renders.<BlockPopoverDropZone>(block-popover/drop-zone.js) is where the presentational drop zone component is.