Hide drop indicator where block isn't allowed to drop. #56843
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,30 +5,47 @@ import { store as blocksStore } from '@wordpress/blocks'; | |
| import { Draggable } from '@wordpress/components'; | ||
| import { useSelect, useDispatch } from '@wordpress/data'; | ||
| import { useEffect, useRef } from '@wordpress/element'; | ||
| import { throttle } from '@wordpress/compose'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import BlockDraggableChip from './draggable-chip'; | ||
| import useScrollWhenDragging from './use-scroll-when-dragging'; | ||
| import { store as blockEditorStore } from '../../store'; | ||
| import { __unstableUseBlockRef as useBlockRef } from '../block-list/use-block-props/use-block-refs'; | ||
| import { isDropTargetValid } from '../use-block-drop-zone'; | ||
|
|
||
| const BlockDraggable = ( { | ||
| children, | ||
| clientIds, | ||
| cloneClassname, | ||
| onDragStart, | ||
| onDragEnd, | ||
| fadeWhenDisabled = false, | ||
| } ) => { | ||
| const { | ||
| srcRootClientId, | ||
| isDraggable, | ||
| icon, | ||
| visibleInserter, | ||
| getBlockType, | ||
| getAllowedBlocks, | ||
| draggedBlockNames, | ||
| getBlockNamesByClientId, | ||
| } = useSelect( | ||
| ( select ) => { | ||
| const { | ||
| canMoveBlocks, | ||
| getBlockRootClientId, | ||
| getBlockName, | ||
| getBlockAttributes, | ||
| isBlockInsertionPointVisible, | ||
| getDraggedBlockClientIds, | ||
| getBlockNamesByClientId: _getBlockNamesByClientId, | ||
| getAllowedBlocks: _getAllowedBlocks, | ||
| } = select( blockEditorStore ); | ||
| const { getBlockType: _getBlockType, getActiveBlockVariation } = | ||
| select( blocksStore ); | ||
| const rootClientId = getBlockRootClientId( clientIds[ 0 ] ); | ||
| const blockName = getBlockName( clientIds[ 0 ] ); | ||
|
|
@@ -40,11 +57,19 @@ const BlockDraggable = ( { | |
| return { | ||
| srcRootClientId: rootClientId, | ||
| isDraggable: canMoveBlocks( clientIds, rootClientId ), | ||
| icon: variation?.icon || _getBlockType( blockName )?.icon, | ||
| visibleInserter: isBlockInsertionPointVisible(), | ||
| getBlockType: _getBlockType, | ||
| getAllowedBlocks: _getAllowedBlocks, | ||
| draggedBlockNames: _getBlockNamesByClientId( | ||
| getDraggedBlockClientIds() | ||
| ), | ||
| getBlockNamesByClientId: _getBlockNamesByClientId, | ||
| }; | ||
| }, | ||
| [ clientIds ] | ||
| ); | ||
|
|
||
| const isDragging = useRef( false ); | ||
| const [ startScrolling, scrollOnDragOver, stopScrolling ] = | ||
| useScrollWhenDragging(); | ||
|
|
@@ -61,6 +86,68 @@ const BlockDraggable = ( { | |
| }; | ||
| }, [] ); | ||
|
|
||
| // Find the root of the editor iframe. | ||
| const blockRef = useBlockRef( clientIds[ 0 ] ); | ||
| const editorRoot = blockRef.current?.closest( 'body' ); | ||
|
|
||
| // Add a dragover event listener to the editor root to track the blocks being dragged over. | ||
| // The listener has to be inside the editor iframe otherwise the target isn't accessible. | ||
| // Check if the dragged blocks are allowed inside the target. If not, grey out the draggable. | ||
| useEffect( () => { | ||
| if ( ! editorRoot ) { | ||
| return; | ||
| } | ||
|
|
||
| const onDragOver = ( event ) => { | ||
|
There was a problem hiding this comment. I think this callback might need to be throttled similar to the code in 2023-12-14.10.37.55.mp4There was a problem hiding this comment. Hrm yeah I'm wondering how best to do that given the inability to use hooks inside the effect callback 🤔 There was a problem hiding this comment. I think I ran into this issue when testing, it seemed to crash in Safari and I couldn't move the chip any more. There was a problem hiding this comment. Oo wait I see there's a There was a problem hiding this comment. Ok the throttle seems to make it better! I set it to the same as draggable. There was a problem hiding this comment. That's a little better, but it's still lagging a bit for me with the throttle at
Would it work to maybe move defining the There was a problem hiding this comment. So I had a think about this 😄 and realised that we actually don't need to use local state for I also tried increasing the throttle time and it doesn't seem to make things any worse, so might leave it at 200 or so. I just pushed an update, let me know if it improves things at all! There was a problem hiding this comment. Nice work! That appears to have fixed up the performance issues for me. Subjectively, without CPU throttling, it's feeling smooth now, and with CPU throttling, it seems much the same as |
||
| if ( ! event.target.closest( '[data-block]' ) ) { | ||
| return; | ||
| } | ||
|
|
||
| const targetClientId = event.target | ||
| .closest( '[data-block]' ) | ||
| .getAttribute( 'data-block' ); | ||
|
|
||
| const allowedBlocks = getAllowedBlocks( targetClientId ); | ||
| const targetBlockName = getBlockNamesByClientId( [ | ||
| targetClientId, | ||
| ] )[ 0 ]; | ||
| const dropTargetValid = isDropTargetValid( | ||
| getBlockType, | ||
| allowedBlocks, | ||
| draggedBlockNames, | ||
| targetBlockName | ||
| ); | ||
|
|
||
| // Update the body class to reflect if drop target is valid. | ||
| // This has to be done on the document body because the draggable | ||
| // chip is rendered outside of the editor iframe. | ||
| if ( ! dropTargetValid && ! visibleInserter ) { | ||
| window?.document?.body?.classList?.add( | ||
| 'block-draggable-invalid-drag-token' | ||
| ); | ||
| } else { | ||
| window?.document?.body?.classList?.remove( | ||
| 'block-draggable-invalid-drag-token' | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| const throttledOnDragOver = throttle( onDragOver, 200 ); | ||
|
|
||
| editorRoot.addEventListener( 'dragover', throttledOnDragOver ); | ||
|
|
||
| return () => { | ||
| editorRoot.removeEventListener( 'dragover', throttledOnDragOver ); | ||
| }; | ||
| }, [ | ||
| draggedBlockNames, | ||
| editorRoot, | ||
| getAllowedBlocks, | ||
| getBlockNamesByClientId, | ||
| getBlockType, | ||
| visibleInserter, | ||
| ] ); | ||
|
|
||
| if ( ! isDraggable ) { | ||
| return children( { draggable: false } ); | ||
| } | ||
|
|
@@ -102,7 +189,14 @@ const BlockDraggable = ( { | |
| } | ||
| } } | ||
| __experimentalDragComponent={ | ||
| <BlockDraggableChip | ||
| count={ clientIds.length } | ||
| icon={ icon } | ||
| className={ | ||
| fadeWhenDisabled && | ||
| 'block-editor-block-draggable-chip-fade' | ||
| } | ||
| /> | ||
| } | ||
| > | ||
| { ( { onDraggableStart, onDraggableEnd } ) => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this
useEffectand itsonDragOverstill needed? Since theuseBlockDropZoneis hiding the insertion point, could we replace the whole useEffect with something like:Then the logic of when to show and hide the insertion point could live in one place, so that the
BlockDraggabledoesn't have to also perform checks with its own drag event handler.Or is there another reason why this component needs to perform its own separate checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so the crucial bit here is finding the
targetClientId, which tells us the container that the draggable is currently hovering over. To find that we need adragoverevent listener attached to the inside of the editor iframe (if it's on the outside, it won't be able to access the elements inside the iframe). We need thetargetClientIdto check if the drop target is valid, which is necessary because if the chip is hovering over a valid container but there's no drop indicator currently showing (if we're moving between drop areas for instance), we don't want it to grey out.Unfortunately there's not much that can be done to restrict how many times this runs without slowing down the responsiveness of the drag chip to what it's hovering over. The problem here is that, unlike with the drop indicators that exist statically between all blocks so we know where they're placed from the start, we need to find out what the draggable is being dragged over, and afaik the only way to do that is through the event target of a
dragoverlistener.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, gotcha! Thanks for the explanation, that makes sense now 👍
Yeah, it can be really tricky to get the right balance when slowing things down so that it's slow enough for performance reasons, but fast enough to feel responsive to a user! 😅
That sounds about right to me, too. If it winds up being a bit of a rabbithole trying to get it working smoothly, would it be worth splitting out the
useBlockDropZonechange and landing that first? Totally cool if you want to keep it all together while figuring everything out of course!