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

Drop indicator: show around dragged block and show above selected block for file drop #31896

Merged
merged 7 commits into from
Jun 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,28 @@ import { InsertionPointOpenRef } from '../block-tools/insertion-point';

export function useInBetweenInserter() {
const openRef = useContext( InsertionPointOpenRef );
const hasReducedUI = useSelect(
( select ) => select( blockEditorStore ).getSettings().hasReducedUI,
[]
);
const {
getBlockListSettings,
getBlockRootClientId,
getBlockIndex,
isBlockInsertionPointVisible,
isMultiSelecting,
getSelectedBlockClientIds,
} = useSelect( blockEditorStore );
const { showInsertionPoint, hideInsertionPoint } = useDispatch(
blockEditorStore
);

return useRefEffect(
( node ) => {
if ( hasReducedUI ) {
return;
}

function onMouseMove( event ) {
if ( openRef.current ) {
return;
Expand Down Expand Up @@ -98,6 +107,12 @@ export function useInBetweenInserter() {
return;
}

// Don't show the inserter when hovering above (conflicts with
// block toolbar) or inside selected block(s).
if ( getSelectedBlockClientIds().includes( clientId ) ) {
return;
}

const elementRect = element.getBoundingClientRect();

if (
Expand Down Expand Up @@ -145,6 +160,7 @@ export function useInBetweenInserter() {
isMultiSelecting,
showInsertionPoint,
hideInsertionPoint,
getSelectedBlockClientIds,
]
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,24 @@ function BlockPopover( {
hasFixedToolbar,
lastClientId,
} = useSelect( selector, [] );
const isInsertionPointVisible = useSelect(
( select ) => {
const {
isBlockInsertionPointVisible,
getBlockInsertionPoint,
getBlockOrder,
} = select( blockEditorStore );

if ( ! isBlockInsertionPointVisible() ) {
return false;
}

const insertionPoint = getBlockInsertionPoint();
const order = getBlockOrder( insertionPoint.rootClientId );
return order[ insertionPoint.index ] === clientId;
},
[ clientId ]
);
const isLargeViewport = useViewportMatch( 'medium' );
const [ isToolbarForced, setIsToolbarForced ] = useState( false );
const [ isInserterShown, setIsInserterShown ] = useState( false );
Expand Down Expand Up @@ -184,7 +202,9 @@ function BlockPopover( {
position={ popoverPosition }
focusOnMount={ false }
anchorRef={ anchorRef }
className="block-editor-block-list__block-popover"
className={ classnames( 'block-editor-block-list__block-popover', {
'is-insertion-point-visible': isInsertionPointVisible,
} ) }
__unstableStickyBoundaryElement={ stickyBoundaryElement }
// Render in the old slot if needed for backward compatibility,
// otherwise render in place (not in the the default popover slot).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ function InsertionPointPopover( {
const ref = useRef();
const {
orientation,
isHidden,
previousClientId,
nextClientId,
rootClientId,
Expand All @@ -45,47 +44,36 @@ function InsertionPointPopover( {
const {
getBlockOrder,
getBlockListSettings,
getMultiSelectedBlockClientIds,
getSelectedBlockClientId,
hasMultiSelection,
getSettings,
getBlockInsertionPoint,
isBlockBeingDragged,
getPreviousBlockClientId,
getNextBlockClientId,
} = select( blockEditorStore );
const insertionPoint = getBlockInsertionPoint();
const order = getBlockOrder( insertionPoint.rootClientId );
const targetClientId = order[ insertionPoint.index - 1 ];
const targetRootClientId = insertionPoint.rootClientId;
const blockOrder = getBlockOrder( targetRootClientId );
if ( ! blockOrder.length ) {

if ( ! order.length ) {
return {};
}
const previous = targetClientId
? targetClientId
: blockOrder[ blockOrder.length - 1 ];
const isLast = previous === blockOrder[ blockOrder.length - 1 ];
const next = isLast
? null
: blockOrder[ blockOrder.indexOf( previous ) + 1 ];
const { hasReducedUI } = getSettings();
const multiSelectedBlockClientIds = getMultiSelectedBlockClientIds();
const selectedBlockClientId = getSelectedBlockClientId();
const blockOrientation =
getBlockListSettings( targetRootClientId )?.orientation ||
'vertical';

let _previousClientId = order[ insertionPoint.index - 1 ];
let _nextClientId = order[ insertionPoint.index ];

while ( isBlockBeingDragged( _previousClientId ) ) {
_previousClientId = getPreviousBlockClientId( _previousClientId );
}

while ( isBlockBeingDragged( _nextClientId ) ) {
_nextClientId = getNextBlockClientId( _nextClientId );
}

return {
previousClientId: previous,
nextClientId: next,
isHidden:
hasReducedUI ||
( hasMultiSelection()
? next && multiSelectedBlockClientIds.includes( next )
: next &&
blockOrientation === 'vertical' &&
next === selectedBlockClientId ),
Comment on lines -81 to -85
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this multi-selection logic no longer needed? I'm not sure what it does ... does it stop the insertertion point from being shown between multi-selected blocks?

That seems to still work when testing, so if that's the case this comment can be ignored.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's no longer needed because now we check in the in-between inserter hook whether to display it. This component should be a bit more flexible. I do think we'd want e.g. a drop indicator to show, just not the in-between inserter when hovering between selected blocks. Even that is debatable. I think it was done to avoid interference when you drag to select blocks.

orientation: blockOrientation,
clientId: targetClientId,
rootClientId: targetRootClientId,
previousClientId: _previousClientId,
nextClientId: _nextClientId,
orientation:
getBlockListSettings( insertionPoint.rootClientId )
?.orientation || 'vertical',
rootClientId: insertionPoint.rootClientId,
isInserterShown: insertionPoint?.__unstableWithInserter,
};
}, [] );
Expand Down Expand Up @@ -193,14 +181,7 @@ function InsertionPointPopover( {
// Only show the inserter when there's a `nextElement` (a block after the
// insertion point). At the end of the block list the trailing appender
// should serve the purpose of inserting blocks.
const showInsertionPointInserter =
! isHidden && nextElement && isInserterShown;

// Show the indicator if the insertion point inserter is visible, or if
// the `showInsertionPoint` state is `true`. The latter is generally true
// when hovering blocks for insertion in the block library.
const showInsertionPointIndicator =
showInsertionPointInserter || ! isHidden;
const showInsertionPointInserter = nextElement && isInserterShown;

/* eslint-disable jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events */
// While ideally it would be enough to capture the
Expand Down Expand Up @@ -231,9 +212,7 @@ function InsertionPointPopover( {
} ) }
style={ style }
>
{ showInsertionPointIndicator && (
<div className="block-editor-block-list__insertion-point-indicator" />
) }
<div className="block-editor-block-list__insertion-point-indicator" />
{ showInsertionPointInserter && (
<div
className={ classnames(
Expand Down Expand Up @@ -266,11 +245,7 @@ export default function InsertionPoint( {
__unstableContentRef,
} ) {
const isVisible = useSelect( ( select ) => {
const { isMultiSelecting, isBlockInsertionPointVisible } = select(
blockEditorStore
);

return isBlockInsertionPointVisible() && ! isMultiSelecting();
return select( blockEditorStore ).isBlockInsertionPointVisible();
}, [] );

return (
Expand Down
5 changes: 5 additions & 0 deletions packages/block-editor/src/components/block-tools/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,11 @@
}
}

// Hide the block toolbar if the insertion point is shown.
&.is-insertion-point-visible {
visibility: hidden;
}

.is-dragging-components-draggable & {
opacity: 0;
// Use a minimal duration to delay hiding the element, see hide-during-dragging animation for more details.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,7 @@ export function getNearestBlockIndex( elements, position, orientation ) {
// If the user is dropping to the trailing edge of the block
// add 1 to the index to represent dragging after.
const isTrailingEdge = edge === 'bottom' || edge === 'right';
let offset = isTrailingEdge ? 1 : 0;

// If the target is the dragged block itself and another 1 to
// index as the dragged block is set to `display: none` and
// should be skipped in the calculation.
const isTargetDraggedBlock =
isTrailingEdge &&
elements[ index + 1 ] &&
elements[ index + 1 ].classList.contains( 'is-dragging' );
offset += isTargetDraggedBlock ? 1 : 0;
const offset = isTrailingEdge ? 1 : 0;

// Update the currently known best candidate.
candidateDistance = distance;
Expand Down Expand Up @@ -144,6 +135,11 @@ export default function useBlockDropZone( {
// https://developer.mozilla.org/en-US/docs/Web/API/Event/currentTarget
throttled( event, event.currentTarget );
},
onDragLeave() {
throttled.cancel();
hideInsertionPoint();
setTargetBlockIndex( null );
},
onDragEnd() {
throttled.cancel();
hideInsertionPoint();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,27 +209,6 @@ describe( 'getNearestBlockIndex', () => {

expect( result ).toBe( 4 );
} );

it( 'skips the block being dragged by checking for the `is-dragging` classname', () => {
const position = { x: 0, y: 450 };

const verticalElementsWithDraggedBlock = [
...verticalElements.slice( 0, 2 ),
{
...verticalElements[ 2 ],
classList: createMockClassList( 'wp-block is-dragging' ),
},
...verticalElements.slice( 3, 4 ),
];

const result = getNearestBlockIndex(
verticalElementsWithDraggedBlock,
position,
orientation
);

expect( result ).toBe( 3 );
} );
} );

describe( 'Horizontal block lists', () => {
Expand Down Expand Up @@ -342,26 +321,5 @@ describe( 'getNearestBlockIndex', () => {

expect( result ).toBe( 4 );
} );

it( 'skips the block being dragged by checking for the `is-dragging` classname', () => {
const position = { x: 450, y: 0 };

const horizontalElementsWithDraggedBlock = [
...horizontalElements.slice( 0, 2 ),
{
...horizontalElements[ 2 ],
classList: createMockClassList( 'wp-block is-dragging' ),
},
...horizontalElements.slice( 3, 4 ),
];

const result = getNearestBlockIndex(
horizontalElementsWithDraggedBlock,
position,
orientation
);

expect( result ).toBe( 3 );
} );
} );
} );
47 changes: 38 additions & 9 deletions packages/compose/src/hooks/use-drop-zone/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,30 @@ export default function useDropZone( {

const { ownerDocument } = element;

/**
* Checks if an element is in the drop zone.
*
* @param {HTMLElement|null} elementToCheck
*
* @return {boolean} True if in drop zone, false if not.
*/
function isElementInZone( elementToCheck ) {
if (
! elementToCheck ||
! element.contains( elementToCheck )
) {
return false;
}

do {
if ( elementToCheck.dataset.isDropZone ) {
return elementToCheck === element;
}
} while ( ( elementToCheck = elementToCheck.parentElement ) );

return false;
}

function maybeDragStart( /** @type {DragEvent} */ event ) {
if ( isDragging ) {
return;
Expand All @@ -82,6 +106,13 @@ export default function useDropZone( {
maybeDragStart
);

// Note that `dragend` doesn't fire consistently for file and
// HTML drag events where the drag origin is outside the browser
// window. In Firefox it may also not fire if the originating
// node is removed.
ownerDocument.addEventListener( 'dragend', maybeDragEnd );
ownerDocument.addEventListener( 'mousemove', maybeDragEnd );

if ( onDragStartRef.current ) {
onDragStartRef.current( event );
}
Expand Down Expand Up @@ -125,8 +156,8 @@ export default function useDropZone( {
// (element that has been entered) should be outside the drop
// zone.
if (
element.contains(
/** @type {Node} */ ( event.relatedTarget )
isElementInZone(
/** @type {HTMLElement|null} */ ( event.relatedTarget )
)
) {
return;
Expand Down Expand Up @@ -168,33 +199,31 @@ export default function useDropZone( {
isDragging = false;

ownerDocument.addEventListener( 'dragenter', maybeDragStart );
ownerDocument.removeEventListener( 'dragend', maybeDragEnd );
ownerDocument.removeEventListener( 'mousemove', maybeDragEnd );

if ( onDragEndRef.current ) {
onDragEndRef.current( event );
}
}

element.dataset.isDropZone = 'true';
element.addEventListener( 'drop', onDrop );
element.addEventListener( 'dragenter', onDragEnter );
element.addEventListener( 'dragover', onDragOver );
element.addEventListener( 'dragleave', onDragLeave );
// Note that `dragend` doesn't fire consistently for file and HTML
// drag events where the drag origin is outside the browser window.
// In Firefox it may also not fire if the originating node is
// removed.
ownerDocument.addEventListener( 'dragend', maybeDragEnd );
ownerDocument.addEventListener( 'mouseup', maybeDragEnd );
// The `dragstart` event doesn't fire if the drag started outside
// the document.
ownerDocument.addEventListener( 'dragenter', maybeDragStart );

return () => {
delete element.dataset.isDropZone;
element.removeEventListener( 'drop', onDrop );
element.removeEventListener( 'dragenter', onDragEnter );
element.removeEventListener( 'dragover', onDragOver );
element.removeEventListener( 'dragleave', onDragLeave );
ownerDocument.removeEventListener( 'dragend', maybeDragEnd );
ownerDocument.removeEventListener( 'mouseup', maybeDragEnd );
ownerDocument.removeEventListener( 'mousemove', maybeDragEnd );
ownerDocument.addEventListener( 'dragenter', maybeDragStart );
};
},
Expand Down