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.

Sign up for GitHub

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

Jump to bottom

List View: Allow dragging outside the immediate area by passing down a dropZoneElement #50726

Merged
merged 13 commits into from
May 24, 2023
Merged

List View: Allow dragging outside the immediate area by passing down a dropZoneElement #50726

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0480537
List View: Allow dragging outside the immediate area by passing down …
andrewserong May 18, 2023
d925f68
Fix nesting logic so it will only nest if hovering over the bottom ha…
andrewserong May 18, 2023
f501706
Update widget sidebar, too
andrewserong May 19, 2023
9d55b5c
Update comments
andrewserong May 19, 2023
65ffea3
Update comments and types
andrewserong May 19, 2023
868c57e
Use setTarget directly in the callback
andrewserong May 19, 2023
970217e
Fix issue where sometimes the expanded drop zone area was not being r…
andrewserong May 22, 2023
84ac583
Add tests for useDropZone
andrewserong May 22, 2023
4eb834d
Update dependency array, we only need to look at the current key
andrewserong May 23, 2023
81b78ee
Swap out dropZoneRef for dropZoneElement
andrewserong May 23, 2023
c5c1e61
Fix typo
andrewserong May 23, 2023
6b362a8
Add README file
andrewserong May 23, 2023
5ce23fa
Update packages/compose/src/hooks/use-drop-zone/README.md
andrewserong May 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 18 additions & 13 deletions packages/block-editor/src/components/list-view/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,27 +52,30 @@ export const BLOCK_LIST_ITEM_HEIGHT = 36;

/** @typedef {import('react').ComponentType} ComponentType */
/** @typedef {import('react').Ref<HTMLElement>} Ref */
/** @typedef {import('react').RefObject} RefObject */

/**
* Show a hierarchical list of blocks.
*
* @param {Object} props Components props.
* @param {string} props.id An HTML element id for the root element of ListView.
* @param {Array} props.blocks _deprecated_ Custom subset of block client IDs to be used instead of the default hierarchy.
* @param {?boolean} props.showBlockMovers Flag to enable block movers. Defaults to `false`.
* @param {?boolean} props.isExpanded Flag to determine whether nested levels are expanded by default. Defaults to `false`.
* @param {?boolean} props.showAppender Flag to show or hide the block appender. Defaults to `false`.
* @param {?ComponentType} props.blockSettingsMenu Optional more menu substitution. Defaults to the standard `BlockSettingsDropdown` component.
* @param {string} props.rootClientId The client id of the root block from which we determine the blocks to show in the list.
* @param {string} props.description Optional accessible description for the tree grid component.
* @param {?Function} props.onSelect Optional callback to be invoked when a block is selected. Receives the block object that was selected.
* @param {Function} props.renderAdditionalBlockUI Function that renders additional block content UI.
* @param {Ref} ref Forwarded ref
* @param {Object} props Components props.
* @param {string} props.id An HTML element id for the root element of ListView.
* @param {Array} props.blocks _deprecated_ Custom subset of block client IDs to be used instead of the default hierarchy.
* @param {?RefObject<HTMLElement>} props.dropZoneRef Optional ref to be used as the drop zone.
* @param {?boolean} props.showBlockMovers Flag to enable block movers. Defaults to `false`.
* @param {?boolean} props.isExpanded Flag to determine whether nested levels are expanded by default. Defaults to `false`.
* @param {?boolean} props.showAppender Flag to show or hide the block appender. Defaults to `false`.
* @param {?ComponentType} props.blockSettingsMenu Optional more menu substitution. Defaults to the standard `BlockSettingsDropdown` component.
* @param {string} props.rootClientId The client id of the root block from which we determine the blocks to show in the list.
* @param {string} props.description Optional accessible description for the tree grid component.
* @param {?Function} props.onSelect Optional callback to be invoked when a block is selected. Receives the block object that was selected.
* @param {Function} props.renderAdditionalBlockUI Function that renders additional block content UI.
* @param {Ref} ref Forwarded ref
*/
function ListViewComponent(
{
id,
blocks,
dropZoneRef: customDropZoneRef,
showBlockMovers = false,
isExpanded = false,
showAppender = false,
Expand Down Expand Up @@ -123,7 +126,9 @@ function ListViewComponent(

const [ expandedState, setExpandedState ] = useReducer( expanded, {} );

const { ref: dropZoneRef, target: blockDropTarget } = useListViewDropZone();
const { ref: dropZoneRef, target: blockDropTarget } = useListViewDropZone( {
dropZoneRef: customDropZoneRef,
} );
const elementRef = useRef();
const treeGridRef = useMergeRefs( [ elementRef, dropZoneRef, ref ] );

Expand Down
17 changes: 17 additions & 0 deletions packages/block-editor/src/components/list-view/test/use-list-view-drop-zone.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,4 +261,21 @@ describe( 'getListViewDropTarget', () => {

expect( target ).toBeUndefined();
} );

it( 'should move below, and not nest when dragging lower than the bottom-most block', () => {
const singleBlock = [ { ...blocksData[ 0 ], innerBlockCount: 0 } ];

// This position is to the right of the block, but below the bottom of the block.
// This should result in the block being moved below the bottom-most block, and
// not being treated as a nesting gesture.
const position = { x: 160, y: 250 };
const target = getListViewDropTarget( singleBlock, position );

expect( target ).toEqual( {
blockIndex: 1,
clientId: 'block-1',
dropPosition: 'bottom',
rootClientId: '',
} );
} );
} );
20 changes: 17 additions & 3 deletions packages/block-editor/src/components/list-view/use-list-view-drop-zone.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,8 @@ function getNextNonDraggedBlock( blocksData, index ) {
* inner block.
*
* Determined based on nesting level indentation of the current block, plus
* the indentation of the next level of nesting.
* the indentation of the next level of nesting. The vertical position of the
* cursor must also be within the block.
*
* @param {WPPoint} point The point representing the cursor position when dragging.
* @param {DOMRect} rect The rectangle.
Expand All @@ -161,7 +162,10 @@ function getNextNonDraggedBlock( blocksData, index ) {
function isNestingGesture( point, rect, nestingLevel = 1 ) {
const blockIndentPosition =
rect.left + nestingLevel * NESTING_LEVEL_INDENTATION;
return point.x > blockIndentPosition + NESTING_LEVEL_INDENTATION;
return (
point.x > blockIndentPosition + NESTING_LEVEL_INDENTATION &&
point.y < rect.bottom
);
}

// Block navigation is always a vertical list, so only allow dropping
Expand Down Expand Up @@ -356,12 +360,17 @@ export function getListViewDropTarget( blocksData, position ) {
};
}

/** @typedef {import('react').RefObject} RefObject */

/**
* A react hook for implementing a drop zone in list view.
*
* @param {Object} props Named parameters.
* @param {?RefObject<HTMLElement>} [props.dropZoneRef] Optional ref to be used as the drop zone.
*
* @return {WPListViewDropZoneTarget} The drop target.
*/
export default function useListViewDropZone() {
export default function useListViewDropZone( { dropZoneRef } ) {
const {
getBlockRootClientId,
getBlockIndex,
Expand Down Expand Up @@ -432,7 +441,12 @@ export default function useListViewDropZone() {
);

const ref = useDropZone( {
dropZoneRef,
onDrop: onBlockDrop,
onDragLeave() {
throttled.cancel();
setTarget( null );
},
onDragOver( event ) {
// `currentTarget` is only available while the event is being
// handled, so get it now and pass it to the thottled function.
Expand Down
22 changes: 13 additions & 9 deletions packages/compose/src/hooks/use-drop-zone/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,18 +33,20 @@ function useFreshRef( value ) {
/**
* A hook to facilitate drag and drop handling.
*
* @param {Object} props Named parameters.
* @param {boolean} [props.isDisabled] Whether or not to disable the drop zone.
* @param {(e: DragEvent) => void} [props.onDragStart] Called when dragging has started.
* @param {(e: DragEvent) => void} [props.onDragEnter] Called when the zone is entered.
* @param {(e: DragEvent) => void} [props.onDragOver] Called when the zone is moved within.
* @param {(e: DragEvent) => void} [props.onDragLeave] Called when the zone is left.
* @param {(e: MouseEvent) => void} [props.onDragEnd] Called when dragging has ended.
* @param {(e: DragEvent) => void} [props.onDrop] Called when dropping in the zone.
* @param {Object} props Named parameters.
* @param {?import('react').RefObject<HTMLElement>} [props.dropZoneRef] Optional ref to be used as the drop zone.
* @param {boolean} [props.isDisabled] Whether or not to disable the drop zone.
* @param {(e: DragEvent) => void} [props.onDragStart] Called when dragging has started.
* @param {(e: DragEvent) => void} [props.onDragEnter] Called when the zone is entered.
* @param {(e: DragEvent) => void} [props.onDragOver] Called when the zone is moved within.
* @param {(e: DragEvent) => void} [props.onDragLeave] Called when the zone is left.
* @param {(e: MouseEvent) => void} [props.onDragEnd] Called when dragging has ended.
* @param {(e: DragEvent) => void} [props.onDrop] Called when dropping in the zone.
*
* @return {import('react').RefCallback<HTMLElement>} Ref callback to be passed to the drop zone element.
*/
export default function useDropZone( {
dropZoneRef,
isDisabled,
onDrop: _onDrop,
onDragStart: _onDragStart,
Expand All @@ -61,11 +63,13 @@ export default function useDropZone( {
const onDragOverRef = useFreshRef( _onDragOver );

return useRefEffect(
( element ) => {
( elem ) => {
if ( isDisabled ) {
return;
}

const element = dropZoneRef?.current ?? elem;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering how this will work in terms of the cleanup of the hook.

Because it's a ref effect, I think the callback will typically only trigger when elem is unmounted, not when dropZoneRef.current is unmounted.

Copy link
Contributor Author

@andrewserong andrewserong May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Good question! I think this is working pretty well so far. To confirm I logged out element within the callback, and it appears to be the right element:

image

As far as I can tell the behaviour is something like the following:

  • We pass in a ref to useDropZone, but useDropZone also returns a ref. That ref is still used in the list view, to attach to the TreeGrid component.
  • When TreeGrid unmounts, that results in the cleanup callback being called.
  • Because the cleanup callback was instantiated with a reference to element being the one passed in by dropZoneRef the cleanup callback cleans up the passed in element.
  • Also, because we have dropZoneRef?.current in the dependencies array of useRefEffect when the passed in ref changes, the cleanup appears to correctly fire, too. From checking the docs, this appears to be consistent with how useEffect works in that the cleanup fires not only when the component unmounts, but whenever any of the dependencies change. The doc comment for useRefEffect appears to confirm this here, too:

    gutenberg/packages/compose/src/hooks/use-ref-effect/index.ts

    Lines 11 to 29 in 62b3b0e

    /**
    * Effect-like ref callback. Just like with `useEffect`, this allows you to
    * return a cleanup function to be run if the ref changes or one of the
    * dependencies changes. The ref is provided as an argument to the callback
    * functions. The main difference between this and `useEffect` is that
    * the `useEffect` callback is not called when the ref changes, but this is.
    * Pass the returned ref callback as the component's ref and merge multiple refs
    * with `useMergeRefs`.
    *
    * It's worth noting that if the dependencies array is empty, there's not
    * strictly a need to clean up event handlers for example, because the node is
    * to be removed. It *is* necessary if you add dependencies because the ref
    * callback will be called multiple times for the same node.
    *
    * @param callback Callback with ref as argument.
    * @param dependencies Dependencies of the callback.
    *
    * @return Ref callback.
    */

So, all up, I don't think there are any stray event listeners not being removed, as far as I can tell.

Do let me know if you think I've missed anything here, though!

Copy link
Contributor

@talldan talldan May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

When TreeGrid unmounts, that results in the cleanup callback being called.

Sorry if I'm making this more complicated. Given this is being introduced as a generic stable API, I'm mainly thinking that there could be different ways it's used (or misused):

  • The dropZoneRef could reference a completely separate part of the DOM that isn't mounted/unmounted at the same time as the effect's component.
  • The dropZoneRef could reference a parent of the effect's component that isn't mounted/unmounted at the same time.

I don't know if there are tests that can be added to assert these different cases work as expected.

It also might be worth expanding upon the docs for the prop to ensure that the right usage of this API is promoted. There's an example of this for Popover's anchor prop that recommends particular usage - https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/popover/README.md#anchor-element--virtualelement--null.

Also, because we have dropZoneRef?.current in the dependencies array of useRefEffect when the passed in ref changes, the cleanup appears to correctly fire, too

This is the part that often trips me up. With dropZoneRef being a ref type, it won't trigger a re-render when it changes. I've sometimes opted to store an element using useState for that reason (which is also what Popover recommends).

Copy link
Contributor Author

@andrewserong andrewserong May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry if I'm making this more complicated.

No apology necessary! I'm happy to go as slowly as needed with this PR — I always like to be a bit more cautious when making changes to lower-level packages such as compose, and it's important we get it right and don't introduce any issues 👍

I don't know if there are tests that can be added to assert these different cases work as expected.

Good example use cases, it's worth digging in, I'll do a little more exploration to see if it's possible to come up with additional test cases. I think expanding the test cases there is probably a good path toward building confidence around the API change.

It also might be worth expanding upon the docs for the prop to ensure that the right usage of this API is promoted. There's an example of this for Popover's anchor prop that recommends particular usage

Good idea, for some reason useDropzone isn't documented in the compose package's readme, but I'll have a go at writing an entry for it.

This is the part that often trips me up. With dropZoneRef being a ref type, it won't trigger a re-render when it changes. I've sometimes opted to store an element using useState for that reason (which is also what Popover recommends).

Ah, good point, because the ref reference itself isn't changing, only current 🤔. In practice, I haven't yet run into any issues in this PR so far. In terms of using useState, do you mean we'd do that internally within useDropZone, or are you imagining that the consumer would use useState, and we'd pass the element directly to useDropZone instead of a ref?

Edit: I think you probably mean the latter — and I can see that the url popover for example, uses the state approach here:

gutenberg/packages/block-editor/src/components/url-popover/image-url-input-ui.js

Line 251 in 493eac0

ref={ setPopoverAnchor }
— happy to swap it over to that pattern here 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Edit: I think you probably mean the latter — and I can see that the url popover for example, uses the state approach here

Yep, exactly that 👍

Copy link
Contributor Author

@andrewserong andrewserong May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Update: I've done the following:

  • Swapped out dropZoneRef for a dropZoneElement and updated usage to use state for storing the element before passing it along to the ListView (and then on to useDropZone)
  • Added a README file for useDropZone — because the hook is still currently exported as experimental, I couldn't add directly to the compose package readme. I see a couple of other experimental hooks also include their own README files in the compose package, so this seemed like a decent place to store the documentation for now, while the hook is still "experimental". Happy for any feedback there, I found it a bit challenging coming up with straightforward wording 😅

I haven't looked into exploring adding other tests just yet, but will dig in tomorrow.

Thanks again for the feedback!

Copy link
Contributor

Choose a reason for hiding this comment

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

because the hook is still currently exported as experimental, I couldn't add directly to the compose package readme.

Thanks, didn't realise it was experimental. It must have been that way for ages!

My hunch is that using state to store the element will probably solve the issues, hopefully that's the case.


let isDragging = false;

const { ownerDocument } = element;
Expand Down
2 changes: 1 addition & 1 deletion packages/edit-post/src/components/secondary-sidebar/list-view-sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ export default function ListViewSidebar() {
>
{ tab === 'list-view' && (
<div className="edit-post-editor__list-view-panel-content">
<ListView />
<ListView dropZoneRef={ listViewRef } />
</div>
) }
{ tab === 'outline' && <ListViewOutline /> }
Expand Down
5 changes: 4 additions & 1 deletion packages/edit-site/src/components/secondary-sidebar/list-view-sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
useMergeRefs,
} from '@wordpress/compose';
import { useDispatch } from '@wordpress/data';
import { useRef } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { closeSmall } from '@wordpress/icons';
import { ESCAPE } from '@wordpress/keycodes';
Expand All @@ -27,6 +28,7 @@ export default function ListViewSidebar() {
const focusOnMountRef = useFocusOnMount( 'firstElement' );
const headerFocusReturnRef = useFocusReturn();
const contentFocusReturnRef = useFocusReturn();
const dropZoneRef = useRef();
function closeOnEscape( event ) {
if ( event.keyCode === ESCAPE && ! event.defaultPrevented ) {
setIsListViewOpened( false );
Expand Down Expand Up @@ -54,10 +56,11 @@ export default function ListViewSidebar() {
className="edit-site-editor__list-view-panel-content"
ref={ useMergeRefs( [
contentFocusReturnRef,
dropZoneRef,
focusOnMountRef,
] ) }
>
<PrivateListView />
<PrivateListView dropZoneRef={ dropZoneRef } />
</div>
</div>
);
Expand Down
5 changes: 4 additions & 1 deletion packages/edit-widgets/src/components/secondary-sidebar/list-view-sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
useMergeRefs,
} from '@wordpress/compose';
import { useDispatch } from '@wordpress/data';
import { useRef } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { closeSmall } from '@wordpress/icons';
import { ESCAPE } from '@wordpress/keycodes';
Expand All @@ -24,6 +25,7 @@ export default function ListViewSidebar() {
const focusOnMountRef = useFocusOnMount( 'firstElement' );
const headerFocusReturnRef = useFocusReturn();
const contentFocusReturnRef = useFocusReturn();
const dropZoneRef = useRef();
function closeOnEscape( event ) {
if ( event.keyCode === ESCAPE && ! event.defaultPrevented ) {
event.preventDefault();
Expand Down Expand Up @@ -52,10 +54,11 @@ export default function ListViewSidebar() {
className="edit-widgets-editor__list-view-panel-content"
ref={ useMergeRefs( [
contentFocusReturnRef,
dropZoneRef,
focusOnMountRef,
] ) }
>
<ListView />
<ListView dropZoneRef={ dropZoneRef } />
</div>
</div>
);
Expand Down