-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Drag and drop: fix drop zones on block drag #67317
Changes from 9 commits
dc20d69
1b66762
83c7b3c
249dc26
16468e8
9c34f2e
e875496
daa7f35
6897d91
74d4822
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ import { __experimentalUseDropZone as useDropZone } from '@wordpress/compose'; | |
/** | ||
* Internal dependencies | ||
*/ | ||
import type { DropType, DropZoneProps } from './types'; | ||
import type { DropZoneProps } from './types'; | ||
import type { WordPressComponentProps } from '../context'; | ||
|
||
/** | ||
|
@@ -47,19 +47,22 @@ export function DropZoneComponent( { | |
onFilesDrop, | ||
onHTMLDrop, | ||
onDrop, | ||
isEligible = () => true, | ||
...restProps | ||
}: WordPressComponentProps< DropZoneProps, 'div', false > ) { | ||
const [ isDraggingOverDocument, setIsDraggingOverDocument ] = | ||
useState< boolean >(); | ||
const [ isDraggingOverElement, setIsDraggingOverElement ] = | ||
useState< boolean >(); | ||
const [ type, setType ] = useState< DropType >(); | ||
const [ isActive, setIsActive ] = useState< boolean >(); | ||
const ref = useDropZone( { | ||
onDrop( event ) { | ||
const files = event.dataTransfer | ||
? getFilesFromDataTransfer( event.dataTransfer ) | ||
: []; | ||
const html = event.dataTransfer?.getData( 'text/html' ); | ||
if ( ! event.dataTransfer ) { | ||
return; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would the event ever not have a dataTransfer? If so, we're changing the behaviour of this function because currently on trunk onDrop gets called if it exists, regardless of dataTransfer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will never be the case, it's just to make TypeScript happy. We already had these conditions before |
||
|
||
const files = getFilesFromDataTransfer( event.dataTransfer ); | ||
const html = event.dataTransfer.getData( 'text/html' ); | ||
|
||
/** | ||
* From Windows Chrome 96, the `event.dataTransfer` returns both file object and HTML. | ||
|
@@ -76,32 +79,31 @@ export function DropZoneComponent( { | |
onDragStart( event ) { | ||
setIsDraggingOverDocument( true ); | ||
|
||
let _type: DropType = 'default'; | ||
if ( ! event.dataTransfer ) { | ||
return; | ||
} | ||
|
||
/** | ||
* From Windows Chrome 96, the `event.dataTransfer` returns both file object and HTML. | ||
* The order of the checks is important to recognize the HTML drop. | ||
*/ | ||
if ( event.dataTransfer?.types.includes( 'text/html' ) ) { | ||
_type = 'html'; | ||
if ( event.dataTransfer.types.includes( 'text/html' ) ) { | ||
setIsActive( !! onHTMLDrop ); | ||
} else if ( | ||
// Check for the types because sometimes the files themselves | ||
// are only available on drop. | ||
event.dataTransfer?.types.includes( 'Files' ) || | ||
( event.dataTransfer | ||
? getFilesFromDataTransfer( event.dataTransfer ) | ||
: [] | ||
).length > 0 | ||
event.dataTransfer.types.includes( 'Files' ) || | ||
getFilesFromDataTransfer( event.dataTransfer ).length > 0 | ||
) { | ||
_type = 'file'; | ||
setIsActive( !! onFilesDrop ); | ||
} else { | ||
setIsActive( !! onDrop && isEligible( event.dataTransfer ) ); | ||
} | ||
|
||
setType( _type ); | ||
}, | ||
onDragEnd() { | ||
setIsDraggingOverElement( false ); | ||
setIsDraggingOverDocument( false ); | ||
setType( undefined ); | ||
setIsActive( undefined ); | ||
}, | ||
onDragEnter() { | ||
setIsDraggingOverElement( true ); | ||
|
@@ -112,14 +114,9 @@ export function DropZoneComponent( { | |
} ); | ||
|
||
const classes = clsx( 'components-drop-zone', className, { | ||
'is-active': | ||
( isDraggingOverDocument || isDraggingOverElement ) && | ||
( ( type === 'file' && onFilesDrop ) || | ||
( type === 'html' && onHTMLDrop ) || | ||
( type === 'default' && onDrop ) ), | ||
'is-active': isActive, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, this looks a lot better! |
||
'is-dragging-over-document': isDraggingOverDocument, | ||
'is-dragging-over-element': isDraggingOverElement, | ||
[ `is-dragging-${ type }` ]: !! type, | ||
} ); | ||
|
||
return ( | ||
|
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.
Won't this always run, because
pattern
is a new object each render? Would it be worth checking equality of its properties instead?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.
Good point, this can be improved. Fixed!