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

Drop zone: rewrite with hooks (useDropZone) #19514

Merged
merged 14 commits into from
Jan 10, 2020
Merged

Drop zone: rewrite with hooks (useDropZone) #19514

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
b082ca6
Drop zone: rewrite with hooks
ellatrix Jan 8, 2020
413e2aa
Add missing drop zones
ellatrix Jan 8, 2020
66e6b91
Use hook for component
ellatrix Jan 8, 2020
464e660
Oops
ellatrix Jan 8, 2020
4032990
fix styling
ellatrix Jan 8, 2020
af10ad6
fix unit tests
ellatrix Jan 8, 2020
3868c40
Simplify
ellatrix Jan 8, 2020
9d3667b
Add style for appender drop target
ellatrix Jan 9, 2020
4011f05
Readd className prop
ellatrix Jan 10, 2020
5f1fc4f
Add dropzone to block list instead of block
ellatrix Jan 10, 2020
a3f8278
Add appender style
ellatrix Jan 10, 2020
b1f2474
Clean up
ellatrix Jan 10, 2020
77cd3da
Mark useDropZone unstable
ellatrix Jan 10, 2020
257a093
Remove position classes
ellatrix Jan 10, 2020
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
145 changes: 51 additions & 94 deletions packages/block-editor/src/components/block-drop-zone/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,13 @@
/**
* External dependencies
*/
import classnames from 'classnames';

/**
* WordPress dependencies
*/
import {
DropZone,
withFilters,
} from '@wordpress/components';
import { useDropZone } from '@wordpress/components';
import {
pasteHandler,
getBlockTransforms,
findTransform,
} from '@wordpress/blocks';
import { Component } from '@wordpress/element';
import { withDispatch, withSelect } from '@wordpress/data';
import { compose } from '@wordpress/compose';
import { useDispatch, useSelect } from '@wordpress/data';

const parseDropEvent = ( event ) => {
let result = {
Expand All @@ -40,49 +30,67 @@ const parseDropEvent = ( event ) => {
return result;
};

class BlockDropZone extends Component {
constructor() {
super( ...arguments );

this.onFilesDrop = this.onFilesDrop.bind( this );
this.onHTMLDrop = this.onHTMLDrop.bind( this );
this.onDrop = this.onDrop.bind( this );
export default function useBlockDropZone( { element, clientId, rootClientId } ) {
function selector( select ) {
const {
getBlockIndex,
getClientIdsOfDescendants,
getSettings,
getTemplateLock,
} = select( 'core/block-editor' );
return {
getBlockIndex,
getClientIdsOfDescendants,
hasUploadPermissions: !! getSettings().mediaUpload,
isLockedAll: getTemplateLock( rootClientId ) === 'all',
};
}

getInsertIndex( position ) {
const { clientId, rootClientId, getBlockIndex } = this.props;
const {
getBlockIndex,
getClientIdsOfDescendants,
hasUploadPermissions,
isLockedAll,
} = useSelect( selector, [ rootClientId ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

What pattern did we settle on? I thought we decided to use inline functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought we settled on allowing it. It's especially handy if variable names are clashing. It's also an optimisation if useSelect has no dependencies, you can move it out of the component. Here we depend on rootClientId though.

const {
insertBlocks,
updateBlockAttributes,
moveBlockToPosition,
} = useDispatch( 'core/block-editor' );

function getInsertIndex( position ) {
if ( clientId !== undefined ) {
const index = getBlockIndex( clientId, rootClientId );
return ( position && position.y === 'top' ) ? index : index + 1;
}
}

onFilesDrop( files, position ) {
if ( ! this.props.hasUploadPermissions ) {
function onFilesDrop( files, position ) {
if ( ! hasUploadPermissions ) {
return;
}

const transformation = findTransform(
getBlockTransforms( 'from' ),
( transform ) => transform.type === 'files' && transform.isMatch( files )
);

if ( transformation ) {
const insertIndex = this.getInsertIndex( position );
const blocks = transformation.transform( files, this.props.updateBlockAttributes );
this.props.insertBlocks( blocks, insertIndex );
const insertIndex = getInsertIndex( position );
const blocks = transformation.transform( files, updateBlockAttributes );
insertBlocks( blocks, insertIndex, rootClientId );
}
}

onHTMLDrop( HTML, position ) {
function onHTMLDrop( HTML, position ) {
const blocks = pasteHandler( { HTML, mode: 'BLOCKS' } );

if ( blocks.length ) {
this.props.insertBlocks( blocks, this.getInsertIndex( position ) );
insertBlocks( blocks, getInsertIndex( position ), rootClientId );
}
}

onDrop( event, position ) {
const { rootClientId: dstRootClientId, clientId: dstClientId, getClientIdsOfDescendants, getBlockIndex } = this.props;
function onDrop( event, position ) {
const { srcRootClientId, srcClientId, srcIndex, type } = parseDropEvent( event );

const isBlockDropType = ( dropType ) => dropType === 'block';
Expand All @@ -95,75 +103,24 @@ class BlockDropZone extends Component {
const isSrcBlockAnAncestorOfDstBlock = ( src, dst ) => getClientIdsOfDescendants( [ src ] ).some( ( id ) => id === dst );

if ( ! isBlockDropType( type ) ||
isSameBlock( srcClientId, dstClientId ) ||
isSrcBlockAnAncestorOfDstBlock( srcClientId, dstClientId || dstRootClientId ) ) {
isSameBlock( srcClientId, clientId ) ||
isSrcBlockAnAncestorOfDstBlock( srcClientId, clientId || rootClientId ) ) {
return;
}

const dstIndex = dstClientId ? getBlockIndex( dstClientId, dstRootClientId ) : undefined;
const positionIndex = this.getInsertIndex( position );
const dstIndex = clientId ? getBlockIndex( clientId, rootClientId ) : undefined;
const positionIndex = getInsertIndex( position );
// If the block is kept at the same level and moved downwards,
// subtract to account for blocks shifting upward to occupy its old position.
const insertIndex = dstIndex && srcIndex < dstIndex && isSameLevel( srcRootClientId, dstRootClientId ) ? positionIndex - 1 : positionIndex;
this.props.moveBlockToPosition( srcClientId, srcRootClientId, insertIndex );
const insertIndex = dstIndex && srcIndex < dstIndex && isSameLevel( srcRootClientId, rootClientId ) ? positionIndex - 1 : positionIndex;
moveBlockToPosition( srcClientId, srcRootClientId, rootClientId, insertIndex );
}

render() {
const { hasUploadPermissions, isLockedAll } = this.props;
if ( isLockedAll ) {
return null;
}
const index = this.getInsertIndex();
const isAppender = index === undefined;
return (
<DropZone
className={ classnames( 'block-editor-block-drop-zone', {
'is-appender': isAppender,
} ) }
onHTMLDrop={ this.onHTMLDrop }
onDrop={ this.onDrop }
onFilesDrop={ hasUploadPermissions ? this.onFilesDrop : undefined }
/>
);
}
return useDropZone( {
element,
onFilesDrop,
onHTMLDrop,
onDrop,
isDisabled: isLockedAll,
} );
}

export default compose(
withDispatch( ( dispatch, ownProps ) => {
const {
insertBlocks,
updateBlockAttributes,
moveBlockToPosition,
} = dispatch( 'core/block-editor' );

return {
insertBlocks( blocks, index ) {
const { rootClientId } = ownProps;

insertBlocks( blocks, index, rootClientId );
},
updateBlockAttributes( ...args ) {
updateBlockAttributes( ...args );
},
moveBlockToPosition( srcClientId, srcRootClientId, dstIndex ) {
const { rootClientId: dstRootClientId } = ownProps;
moveBlockToPosition( srcClientId, srcRootClientId, dstRootClientId, dstIndex );
},
};
} ),
withSelect( ( select, { rootClientId } ) => {
const {
getBlockIndex,
getClientIdsOfDescendants,
getSettings,
getTemplateLock,
} = select( 'core/block-editor' );
return {
getBlockIndex,
getClientIdsOfDescendants,
hasUploadPermissions: !! getSettings().mediaUpload,
isLockedAll: getTemplateLock( rootClientId ) === 'all',
};
} ),
withFilters( 'editor.BlockDropZone' )
)( BlockDropZone );
27 changes: 0 additions & 27 deletions packages/block-editor/src/components/block-drop-zone/style.scss
View file
Open in desktop

This file was deleted.

14 changes: 13 additions & 1 deletion packages/block-editor/src/components/block-list-appender/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,21 @@
* External dependencies
*/
import { last } from 'lodash';
import classnames from 'classnames';

/**
* WordPress dependencies
*/
import { withSelect } from '@wordpress/data';
import { getDefaultBlockName } from '@wordpress/blocks';
import { useRef } from '@wordpress/element';

/**
* Internal dependencies
*/
import DefaultBlockAppender from '../default-block-appender';
import ButtonBlockAppender from '../button-block-appender';
import useBlockDropZone from '../block-drop-zone';

function stopPropagation( event ) {
event.stopPropagation();
Expand All @@ -26,6 +29,12 @@ function BlockListAppender( {
isLocked,
renderAppender: CustomAppender,
} ) {
const ref = useRef();
const { isDraggingOverElement } = useBlockDropZone( {
element: ref,
rootClientId,
} );
ellatrix marked this conversation as resolved.
Show resolved Hide resolved

if ( isLocked || CustomAppender === false ) {
return null;
}
Expand Down Expand Up @@ -68,7 +77,10 @@ function BlockListAppender( {
// Prevent the block from being selected when the appender is
// clicked.
onFocus={ stopPropagation }
className="block-list-appender"
ref={ ref }
className={ classnames( 'block-list-appender', {
'is-dragging-over': isDraggingOverElement,
} ) }
>
{ appender }
</div>
Expand Down
10 changes: 10 additions & 0 deletions packages/block-editor/src/components/block-list-appender/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,16 @@
}
}

.block-list-appender.is-dragging-over > div::before {
content: "";
position: absolute;
right: -$block-padding;
left: -$block-padding;
top: -$block-padding;
bottom: -$block-padding;
border: 3px solid theme(primary);
}

.block-list-appender > .block-editor-inserter {
display: block;
}
22 changes: 9 additions & 13 deletions packages/block-editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import { useShortcut } from '@wordpress/keyboard-shortcuts';
* Internal dependencies
*/
import BlockEdit from '../block-edit';
import BlockDropZone from '../block-drop-zone';
import useBlockDropZone from '../block-drop-zone';
import BlockInvalidWarning from './block-invalid-warning';
import BlockCrashWarning from './block-crash-warning';
import BlockCrashBoundary from './block-crash-boundary';
Expand Down Expand Up @@ -284,16 +284,14 @@ function BlockListBlock( {
isFirstMultiSelected
);

// Insertion point can only be made visible if the block is at the
// the extent of a multi-selection, or not in a multi-selection.
const shouldShowInsertionPoint = ! isMultiSelecting && (
( isPartOfMultiSelection && isFirstMultiSelected ) ||
! isPartOfMultiSelection
);

const shouldRenderDropzone = shouldShowInsertionPoint;
const isDragging = isDraggingBlocks && ( isSelected || isPartOfMultiSelection );

const { position } = useBlockDropZone( {
element: wrapper,
clientId,
rootClientId,
} );
ellatrix marked this conversation as resolved.
Show resolved Hide resolved

// The wp-block className is important for editor styles.
// Generate the wrapper class names handling the different states of the block.
const wrapperClassName = classnames(
Expand All @@ -311,6 +309,8 @@ function BlockListBlock( {
'is-focus-mode': isFocusMode,
'has-child-selected': isAncestorOfSelectedBlock,
'has-toolbar-captured': hasAncestorCapturingToolbars,
'is-dragging-close-to-top': position && position.y === 'top',
'is-dragging-close-to-bottom': position && position.y === 'bottom',
},
className
);
Expand Down Expand Up @@ -394,10 +394,6 @@ function BlockListBlock( {
animationStyle
}
>
{ shouldRenderDropzone && <BlockDropZone
clientId={ clientId }
rootClientId={ rootClientId }
/> }
Copy link
Contributor

Choose a reason for hiding this comment

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

The main problem with this PR I think is that the dropzone is smaller than before. In master for paragraphs, its size is "37px" with this pr it is "30px". They both suffer from the same issue, there's a space (margin) between blocks where you can't drop blocks. It's is more visible in this PR because of the smaller size though.

Copy link
Member Author

Choose a reason for hiding this comment

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

@youknowriad I fixed the issue entirely by rewriting useBlockDropZone to work on BlockList instead of BlockListBlock. Now there are no longer "dead zones" between the blocks where you can't drop.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is cool, I'll test a bit later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I think the error might be gone too, but let me know if it isn't. :)

{ hasAncestorCapturingToolbars && ( shouldShowContextualToolbar || isToolbarForced ) && (
// If the parent Block is set to consume toolbars of the child Blocks
// then render the child Block's toolbar into the Slot provided
Expand Down
15 changes: 8 additions & 7 deletions packages/block-editor/src/components/block-list/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,14 @@
opacity: 1;
}
}

&.is-dragging-close-to-top::before {
border-top: 3px solid theme(primary);
}

&.is-dragging-close-to-bottom::before {
border-bottom: 3px solid theme(primary);
}
}


Expand Down Expand Up @@ -351,13 +359,6 @@
float: none;
}

// Dropzones.
.block-editor-block-drop-zone {
top: -4px;
bottom: -3px;
margin: 0 $block-padding;
}

// This essentially duplicates the mobile styles for the appender component.
// It would be nice to be able to use element queries in that component instead https://github.com/tomhodgins/element-queries-spec
.block-editor-block-list__layout {
Expand Down
Loading