Skip to content

Migrate DragAndDrop/index.js to function component #23252

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

Closed
jczekalski wants to merge 6 commits into from
Closed

Migrate DragAndDrop/index.js to function component #23252

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
eef663c
WIP
jczekalski Jul 19, 2023
a3a8ba5
clean up
jczekalski Jul 20, 2023
002bb90
resolve conflicts
jczekalski Jul 20, 2023
3f67b9d
syntax improvement
jczekalski Jul 20, 2023
f42152f
revert change added for testing purposes
jczekalski Jul 20, 2023
2b58a42
use usePrevious hook instead of useRef
jczekalski Jul 20, 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
237 changes: 121 additions & 116 deletions src/components/DragAndDrop/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,21 @@
import React from 'react';
import {useCallback, useEffect, useRef} from 'react';
import PropTypes from 'prop-types';
import _ from 'underscore';

import variables from '../../styles/variables';
import DragAndDropPropTypes from './dragAndDropPropTypes';
import withNavigationFocus from '../withNavigationFocus';
import usePrevious from '../../hooks/usePrevious';

const COPY_DROP_EFFECT = 'copy';
const NONE_DROP_EFFECT = 'none';

const DRAG_OVER_EVENT = 'dragover';
const DRAG_ENTER_EVENT = 'dragenter';
const DRAG_LEAVE_EVENT = 'dragleave';
const DROP_EVENT = 'drop';
const RESIZE_EVENT = 'resize';

const propTypes = {

@roryabraham roryabraham Jul 21, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also:

  • the disabled prop should be renamed to isDisabled
  • dropZoneId should be renamed to dropZoneID
  • activeDropZoneId should be renamed to activeDropZoneID

...DragAndDropPropTypes,

@roryabraham roryabraham Jul 21, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The onDragOver prop is included in DragAndDropProps so is duplicated on line 16. Let's remove it please.

@roryabraham roryabraham Jul 21, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually, onDragOver is unused by any code consuming this component, so we should just remove it.

@roryabraham roryabraham Jul 21, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, I noticed that DragAndDropPropTypes are not used outside this file...


Expand Down Expand Up @@ -40,80 +47,28 @@ const defaultProps = {
disabled: false,
};

class DragAndDrop extends React.Component {
constructor(props) {
super(props);

this.throttledDragOverHandler = _.throttle(this.dragOverHandler.bind(this), 100);
this.throttledDragNDropWindowResizeListener = _.throttle(this.dragNDropWindowResizeListener.bind(this), 100);
this.dropZoneDragHandler = this.dropZoneDragHandler.bind(this);
this.dropZoneDragListener = this.dropZoneDragListener.bind(this);

/*
function DragAndDrop(props) {

@roryabraham roryabraham Jul 21, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please destructure props here

const prevIsFocused = usePrevious(props.isFocused);
const prevIsDisabled = usePrevious(props.disabled);
const dropZone = useRef(null);
const dropZoneRect = useRef(null);
/*
Last detected drag state on the dropzone -> we start with dragleave since user is not dragging initially.
This state is updated when drop zone is left/entered entirely(not taking the children in the account) or entire window is left
*/
this.dropZoneDragState = 'dragleave';
}

componentDidMount() {
if (this.props.disabled) {
return;
}
this.addEventListeners();
}

componentDidUpdate(prevProps) {
const isDisabled = this.props.disabled;
if (this.props.isFocused === prevProps.isFocused && isDisabled === prevProps.disabled) {
return;
}
if (!this.props.isFocused || isDisabled) {
this.removeEventListeners();
} else {
this.addEventListeners();
}
}

componentWillUnmount() {
if (this.props.disabled) {
return;
}
this.removeEventListeners();
}

addEventListeners() {
this.dropZone = document.getElementById(this.props.dropZoneId);
this.dropZoneRect = this.calculateDropZoneClientReact();
document.addEventListener('dragover', this.dropZoneDragListener);
document.addEventListener('dragenter', this.dropZoneDragListener);
document.addEventListener('dragleave', this.dropZoneDragListener);
document.addEventListener('drop', this.dropZoneDragListener);
window.addEventListener('resize', this.throttledDragNDropWindowResizeListener);
}

removeEventListeners() {
document.removeEventListener('dragover', this.dropZoneDragListener);
document.removeEventListener('dragenter', this.dropZoneDragListener);
document.removeEventListener('dragleave', this.dropZoneDragListener);
document.removeEventListener('drop', this.dropZoneDragListener);
window.removeEventListener('resize', this.throttledDragNDropWindowResizeListener);
}
*/
const dropZoneDragState = useRef(DRAG_LEAVE_EVENT);

/**
* @param {Object} event native Event
*/
dragOverHandler(event) {
this.props.onDragOver(event);
}
const dragOverHandler = (event) => {

@roryabraham roryabraham Jul 21, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function seems kind of useless. Let's just fold it into the throttled one? Also, more importantly, with this implementation _.throttle will get called again with every re-render, and I think that will cause the timer to restart. I think something like this is more what we want:

function DragAndDrop({onDragOver, ...allTheOtherProps}) {
   ...
   ...

   const dragOverHandler = useCallback(_.throttle(onDragOver, 100), [onDragOver]);

   ...
   ...
}

props.onDragOver(event);
};

dragNDropWindowResizeListener() {
// Update bounding client rect on window resize
this.dropZoneRect = this.calculateDropZoneClientReact();
}
const throttledDragOverHandler = _.throttle(dragOverHandler, 100);

calculateDropZoneClientReact() {
const boundingClientRect = this.dropZone.getBoundingClientRect();
const calculateDropZoneClientReact = useCallback(() => {
const boundingClientRect = dropZone.current.getBoundingClientRect();

// Handle edge case when we are under responsive breakpoint the browser doesn't normalize rect.left to 0 and rect.right to window.innerWidth
return {
Expand All @@ -123,72 +78,122 @@ class DragAndDrop extends React.Component {
top: boundingClientRect.top,
bottom: boundingClientRect.bottom,
};
}
}, []);

const dragNDropWindowResizeListener = () => {
// Update bounding client rect on window resize
dropZoneRect.current = calculateDropZoneClientReact();
};

const throttledDragNDropWindowResizeListener = _.throttle(dragNDropWindowResizeListener, 100);

/**
* @param {Object} event native Event
*/
dropZoneDragHandler(event) {
// Setting dropEffect for dragover is required for '+' icon on certain platforms/browsers (eg. Safari)
switch (event.type) {
case 'dragover':
// Continuous event -> can hurt performance, be careful when subscribing
// eslint-disable-next-line no-param-reassign
event.dataTransfer.dropEffect = COPY_DROP_EFFECT;
this.throttledDragOverHandler(event);
break;
case 'dragenter':
// Avoid reporting onDragEnter for children views -> not performant
if (this.dropZoneDragState === 'dragleave') {
this.dropZoneDragState = 'dragenter';
const dropZoneDragHandler = useCallback(
(event) => {
// Setting dropEffect for dragover is required for '+' icon on certain platforms/browsers (eg. Safari)
switch (event.type) {
case DRAG_OVER_EVENT:
// Continuous event -> can hurt performance, be careful when subscribing
// eslint-disable-next-line no-param-reassign
event.dataTransfer.dropEffect = COPY_DROP_EFFECT;
this.props.onDragEnter(event);
}
break;
case 'dragleave':
if (this.dropZoneDragState === 'dragenter') {
if (
event.clientY <= this.dropZoneRect.top ||
event.clientY >= this.dropZoneRect.bottom ||
event.clientX <= this.dropZoneRect.left ||
event.clientX >= this.dropZoneRect.right ||
// Cancel drag when file manager is on top of the drop zone area - works only on chromium
(event.target.getAttribute('id') === this.props.activeDropZoneId && !event.relatedTarget)
) {
this.dropZoneDragState = 'dragleave';
this.props.onDragLeave(event);
throttledDragOverHandler(event);
break;
case DRAG_ENTER_EVENT:
// Avoid reporting onDragEnter for children views -> not performant
if (dropZoneDragState.current === DRAG_LEAVE_EVENT) {
dropZoneDragState.current = DRAG_ENTER_EVENT;
// eslint-disable-next-line no-param-reassign
event.dataTransfer.dropEffect = COPY_DROP_EFFECT;
props.onDragEnter(event);
}
}
break;
case 'drop':
this.dropZoneDragState = 'dragleave';
this.props.onDrop(event);
break;
default:
break;
}
}
break;
case DRAG_LEAVE_EVENT:
if (dropZoneDragState.current === DRAG_ENTER_EVENT) {
if (
event.clientY <= dropZoneRect.current.top ||
event.clientY >= dropZoneRect.current.bottom ||
event.clientX <= dropZoneRect.current.left ||
event.clientX >= dropZoneRect.current.right ||
// Cancel drag when file manager is on top of the drop zone area - works only on chromium
(event.target.getAttribute('id') === props.activeDropZoneId && !event.relatedTarget)
) {
dropZoneDragState.current = DRAG_LEAVE_EVENT;
props.onDragLeave(event);
}
}
break;
case DROP_EVENT:
dropZoneDragState.current = DRAG_LEAVE_EVENT;
props.onDrop(event);
break;
default:
break;
}
},
[props, throttledDragOverHandler],
);

/**
* Handles all types of drag-N-drop events on the drop zone associated with composer
*
* @param {Object} event native Event
*/
dropZoneDragListener(event) {
event.preventDefault();
const dropZoneDragListener = useCallback(
(event) => {
event.preventDefault();

if (this.dropZone.contains(event.target) && this.props.shouldAcceptDrop(event)) {
this.dropZoneDragHandler(event);
if (dropZone.current.contains(event.target) && props.shouldAcceptDrop(event)) {

@roryabraham roryabraham Jul 21, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldAcceptDrop is never passed in as a prop to DragAndDrop AFAICT, so it doesn't really need to be a prop. It can just declare as a pure function outside of the component.

dropZoneDragHandler(event);
} else {
// eslint-disable-next-line no-param-reassign
event.dataTransfer.dropEffect = NONE_DROP_EFFECT;
}
},
[props, dropZoneDragHandler],
);

const addEventListeners = useCallback(() => {
dropZone.current = document.getElementById(props.dropZoneId);
dropZoneRect.current = calculateDropZoneClientReact();
document.addEventListener(DRAG_OVER_EVENT, dropZoneDragListener);
document.addEventListener(DRAG_ENTER_EVENT, dropZoneDragListener);
document.addEventListener(DRAG_LEAVE_EVENT, dropZoneDragListener);
document.addEventListener(DROP_EVENT, dropZoneDragListener);
window.addEventListener(RESIZE_EVENT, throttledDragNDropWindowResizeListener);
}, [props.dropZoneId, calculateDropZoneClientReact, dropZoneDragListener, throttledDragNDropWindowResizeListener]);

const removeEventListeners = useCallback(() => {
document.removeEventListener(DRAG_OVER_EVENT, dropZoneDragListener);
document.removeEventListener(DRAG_ENTER_EVENT, dropZoneDragListener);
document.removeEventListener(DRAG_LEAVE_EVENT, dropZoneDragListener);
document.removeEventListener(DROP_EVENT, dropZoneDragListener);
window.removeEventListener(RESIZE_EVENT, throttledDragNDropWindowResizeListener);
}, [dropZoneDragListener, throttledDragNDropWindowResizeListener]);

useEffect(() => {

@roryabraham roryabraham Jul 21, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's pretty weird how we add and remove event listeners in these effect hooks depending on the isDisabled and isFocused props. I think it would be much simpler to just adjust the event handler so that it accounts for those props, and then adding and removing the event listeners becomes much simpler.

@jczekalski jczekalski Jul 24, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point! I also thought that this was a bit weird, but decided to keep the current implementation since the purpose of the task is just to rewrite the component from class to functional syntax. I'll implement the changes you suggested.

if (props.disabled) {
return;
}
addEventListeners();

return removeEventListeners;
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

useEffect(() => {
if (props.isFocused === prevIsFocused && props.disabled === prevIsDisabled) {
Comment thread
jczekalski marked this conversation as resolved.
return;
}
if (!props.isFocused || props.disabled) {
removeEventListeners();
} else {
// eslint-disable-next-line no-param-reassign
event.dataTransfer.dropEffect = NONE_DROP_EFFECT;
addEventListeners();
}
}
}, [props.disabled, props.isFocused, prevIsDisabled, prevIsFocused, addEventListeners, removeEventListeners]);
Comment thread
jczekalski marked this conversation as resolved.

render() {
return this.props.children;
}
return props.children;
}

DragAndDrop.propTypes = propTypes;
Expand Down