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

Fix for Issue 4329: Updated dropzone provider to check hovered zones on dragover. #4604

Merged
merged 14 commits into from
Feb 1, 2018
Merged

Fix for Issue 4329: Updated dropzone provider to check hovered zones on dragover. #4604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
db732c5
Fix for Issue 4329: Updated dropzone provider to check hovered zone
chriskmnds Jan 19, 2018
e09051d
Fix for Issue 4329: Linting errors.
chriskmnds Jan 19, 2018
97bc6f5
Fix for Issue 4329: Cleanup.
chriskmnds Jan 19, 2018
22013ae
Merge branch 'master' into fix/dropzone-provider
chriskmnds Jan 26, 2018
93ad7ad
Merge branch 'master' into fix/dropzone-provider
chriskmnds Jan 27, 2018
64de0b6
Fix for Issue 4329: Removed inclusion of stopPropagation call in the …
chriskmnds Jan 27, 2018
c33fcfd
Fix for Issue 4329: Cleanup - removed redundant check with undefined …
chriskmnds Jan 29, 2018
eed854f
Fix for Issue 4329: Improved border overlap between dropzones.
chriskmnds Jan 29, 2018
df5f586
Fix for Issue 4329: Added throttling of 500 ms to dropzone provider's…
chriskmnds Jan 31, 2018
25738a0
Fix for Issue 4329: Reduced throttling to 300 ms.
chriskmnds Jan 31, 2018
aa6f3b3
Fix for Issue 4329: Cleanup - linting errors.
chriskmnds Jan 31, 2018
c4c784e
Merge branch 'master' into fix/dropzone-provider
chriskmnds Jan 31, 2018
6f5ac93
Fix for Issue 4329: Cleanup/refactored after Riad's comments + lowere…
chriskmnds Jan 31, 2018
f7cc69e
Fix for Issue 4329: Cleanup/refactored after review.
chriskmnds Jan 31, 2018
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
57 changes: 28 additions & 29 deletions components/drop-zone/provider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { isEqual, without, some, filter, findIndex, noop } from 'lodash';
import { isEqual, without, some, filter, findIndex, noop, throttle } from 'lodash';

/**
* WordPress dependencies
Expand All @@ -13,8 +13,8 @@ class DropZoneProvider extends Component {
super( ...arguments );

this.resetDragState = this.resetDragState.bind( this );
this.toggleDraggingOverDocument = this.toggleDraggingOverDocument.bind( this );
this.onDragOver = this.onDragOver.bind( this );
this.toggleDraggingOverDocument = throttle( this.toggleDraggingOverDocument.bind( this ), 200 );
this.dragOverListener = this.dragOverListener.bind( this );
this.isWithinZoneBounds = this.isWithinZoneBounds.bind( this );
this.onDrop = this.onDrop.bind( this );

Expand All @@ -26,6 +26,11 @@ class DropZoneProvider extends Component {
this.dropzones = [];
}

dragOverListener( event ) {
this.toggleDraggingOverDocument( event );
event.preventDefault();
}

getChildContext() {
return {
dropzones: {
Expand All @@ -40,18 +45,14 @@ class DropZoneProvider extends Component {
}

componentDidMount() {
window.addEventListener( 'dragover', this.onDragOver );
window.addEventListener( 'dragover', this.dragOverListener );
window.addEventListener( 'drop', this.onDrop );
window.addEventListener( 'dragenter', this.toggleDraggingOverDocument );
window.addEventListener( 'dragleave', this.toggleDraggingOverDocument );
window.addEventListener( 'mouseup', this.resetDragState );
}

componentWillUnmount() {
window.removeEventListener( 'dragover', this.onDragOver );
window.removeEventListener( 'dragover', this.dragOverListener );
window.removeEventListener( 'drop', this.onDrop );
window.removeEventListener( 'dragenter', this.toggleDraggingOverDocument );
window.removeEventListener( 'dragleave', this.toggleDraggingOverDocument );
window.removeEventListener( 'mouseup', this.resetDragState );
}

Expand All @@ -76,10 +77,6 @@ class DropZoneProvider extends Component {
} );
}

onDragOver( event ) {
event.preventDefault();
}

toggleDraggingOverDocument( event ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is called so many times when dragging right now, do you think we should throttle it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Great idea Riad. I am looking into this, currently trying something out with lodash. Sorry for the delayed response, I was just away since yesterday.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Riad, I've finally added throttling at 300ms using lodash. It feels ok to me at this value, but please have a go and let me know what you think. Also, this is the best I could come up with code-wise to get the event.preventDefault call working. Tested it in Chrome/Firefox/Safari/IE11. :-)

// In some contexts, it may be necessary to capture and redirect the
// drag event (e.g. atop an `iframe`). To accommodate this, you can
Expand All @@ -89,25 +86,26 @@ class DropZoneProvider extends Component {
// See: https://developer.mozilla.org/en-US/docs/Web/Guide/Events/Creating_and_triggering_events
const detail = window.CustomEvent && event instanceof window.CustomEvent ? event.detail : event;

// Computing state
const { onVerifyValidTransfer } = this.props;
const isValidDrag = ! onVerifyValidTransfer || onVerifyValidTransfer( detail.dataTransfer );
const isDraggingOverDocument = isValidDrag; // && this.dragEnterNodes.length;
const hoveredDropZone = isDraggingOverDocument && findIndex( this.dropzones, ( { element } ) =>
this.isWithinZoneBounds( element, detail.clientX, detail.clientY
) );
// Index of hovered dropzone.
const hoveredDropZone = findIndex( this.dropzones, ( { element } ) =>
this.isWithinZoneBounds( element, detail.clientX, detail.clientY )
);

let position = null;

if ( hoveredDropZone !== -1 ) {
const rect = this.dropzones[ hoveredDropZone ].element.getBoundingClientRect();
position = hoveredDropZone === -1 ? null : {

position = {
x: detail.clientX - rect.left < rect.right - detail.clientX ? 'left' : 'right',
y: detail.clientY - rect.top < rect.bottom - detail.clientY ? 'top' : 'bottom',
};
}

// Optimisation: Only update the changed dropzones
let dropzonesToUpdate = [];
if ( this.state.isDraggingOverDocument !== isDraggingOverDocument ) {

if ( ! this.state.isDraggingOverDocument ) {
dropzonesToUpdate = this.dropzones;
} else if ( hoveredDropZone !== this.state.hoveredDropZone ) {
if ( this.state.hoveredDropZone !== -1 ) {
Expand All @@ -130,14 +128,17 @@ class DropZoneProvider extends Component {
dropzone.updateState( {
isDraggingOverElement: index === hoveredDropZone,
position: index === hoveredDropZone ? position : null,
isDraggingOverDocument,
isDraggingOverDocument: true,
} );
} );

this.setState( {
isDraggingOverDocument,
isDraggingOverDocument: true,
hoveredDropZone,
position,
} );

event.preventDefault();
}

isWithinZoneBounds( dropzone, x, y ) {
Expand All @@ -162,18 +163,16 @@ class DropZoneProvider extends Component {
// This seemingly useless line has been shown to resolve a Safari issue
// where files dragged directly from the dock are not recognized
event.dataTransfer && event.dataTransfer.files.length; // eslint-disable-line no-unused-expressions

Copy link
Member

Choose a reason for hiding this comment

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

Do we need those empty lines?

Copy link
Contributor Author

@chriskmnds chriskmnds Jan 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yes 😛

Copy link
Contributor Author

@chriskmnds chriskmnds Jan 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

jk! In general I prefer to keep declarations (const/let/var/etc) and conditionals separate from other statements. That's just personal preference though - I am more than happy to remove them. Do we have any general approach/convention that I can adopt for such details?

const { position, hoveredDropZone } = this.state;
const dropzone = hoveredDropZone !== -1 ? this.dropzones[ hoveredDropZone ] : null;

this.resetDragState();

if ( !! dropzone && !! dropzone.onDrop ) {
dropzone.onDrop( event, position );
}

const { onVerifyValidTransfer } = this.props;
if ( onVerifyValidTransfer && ! onVerifyValidTransfer( event.dataTransfer ) ) {
return;
}

if ( event.dataTransfer && !! dropzone ) {
const files = event.dataTransfer.files;
const HTML = event.dataTransfer.getData( 'text/html' );
Expand Down
4 changes: 2 additions & 2 deletions editor/components/block-list/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@

// focused block-style
&.is-selected:before {
outline: 1px solid $light-gray-500;
outline: 1px solid $light-gray-500;
}

// give reusable blocks a dashed outline
Expand Down Expand Up @@ -255,7 +255,7 @@
& > .components-drop-zone {
border: none;
top: -4px;
bottom: -4px;
bottom: -3px;
margin: 0 $block-mover-padding-visible;
border-radius: 0;

Expand Down