Migrate DragAndDrop/index.js to function component #23252
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@thesahindia Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
AndrewGable
requested review from
allroundexperts
and removed request for
thesahindia
Reviewer Checklist
Screenshots/VideosWebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
roryabraham
requested changes
Jul 21, 2023
There was a problem hiding this comment.
I left a bunch of comments related to this code in #23046 before I knew that this PR existed. Can we try to address those?
roryabraham
requested changes
Jul 22, 2023
There was a problem hiding this comment.
Ended up having a lot of comments, so I just created a diff with all the changes I'd like to see here. I think the end result ends up being much simpler and cleaner.
diff --git a/src/components/DragAndDrop/DropZone/index.js b/src/components/DragAndDrop/DropZone/index.js
index 525e5a8c14..a073009e22 100644
--- a/src/components/DragAndDrop/DropZone/index.js
+++ b/src/components/DragAndDrop/DropZone/index.js
@@ -12,7 +12,7 @@ const propTypes = {
children: PropTypes.node.isRequired,
/** Required for drag and drop to properly detect dropzone */
- dropZoneId: PropTypes.string.isRequired,
+ dropZoneID: PropTypes.string.isRequired,
};
function DropZone(props) {
@@ -21,7 +21,7 @@ function DropZone(props) {
<View style={[styles.fullScreenTransparentOverlay, styles.alignItemsCenter, styles.justifyContentCenter]}>{props.children}</View>
{/* Necessary for blocking events on content which can publish unwanted dragleave even if we are inside dropzone */}
<View
- nativeID={props.dropZoneId}
+ nativeID={props.dropZoneID}
style={styles.dropZoneTopInvisibleOverlay}
/>
</Portal>
diff --git a/src/components/DragAndDrop/dragAndDropPropTypes.js b/src/components/DragAndDrop/dragAndDropPropTypes.js
index f0b56caeb3..b0720ec99f 100644
--- a/src/components/DragAndDrop/dragAndDropPropTypes.js
+++ b/src/components/DragAndDrop/dragAndDropPropTypes.js
@@ -1,9 +1,6 @@
import PropTypes from 'prop-types';
export default {
- /** Callback to fire when a file has being dragged over the text input & report body */
- onDragOver: PropTypes.func,
-
/** Callback to fire when a file has been dragged into the text input & report body */
onDragEnter: PropTypes.func.isRequired,
@@ -13,12 +10,15 @@ export default {
/** Callback to fire when a file is dropped on the text input & report body */
onDrop: PropTypes.func.isRequired,
- /** Guard for accepting drops in drop zone. Drag event is passed to this function as first parameter */
- shouldAcceptDrop: PropTypes.func,
-
/** Id of the element on which we want to detect drag */
- dropZoneId: PropTypes.string.isRequired,
+ dropZoneID: PropTypes.string.isRequired,
/** Id of the element which is shown while drag is active */
- activeDropZoneId: PropTypes.string.isRequired,
+ activeDropZoneID: PropTypes.string.isRequired,
+
+ /** Whether drag & drop should be disabled */
+ isDisabled: PropTypes.bool,
+
+ /** Rendered child component */
+ children: PropTypes.node.isRequired,
};
diff --git a/src/components/DragAndDrop/index.js b/src/components/DragAndDrop/index.js
index 150137a92f..38356efabd 100644
--- a/src/components/DragAndDrop/index.js
+++ b/src/components/DragAndDrop/index.js
@@ -1,202 +1,135 @@
import {useCallback, useEffect, useRef} from 'react';
-import PropTypes from 'prop-types';
import _ from 'underscore';
+import {useIsFocused} from '@react-navigation/native';
-import variables from '../../styles/variables';
import DragAndDropPropTypes from './dragAndDropPropTypes';
-import withNavigationFocus from '../withNavigationFocus';
-import usePrevious from '../../hooks/usePrevious';
+import useWindowDimensions from '../../hooks/useWindowDimensions';
+import useEffectOnPageLoad from '../../hooks/useEffectOnPageLoad';
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 = {
- ...DragAndDropPropTypes,
-
- /** Callback to fire when a file has being dragged over the text input & report body. This prop is necessary to be inlined to satisfy the linter */
- onDragOver: DragAndDropPropTypes.onDragOver,
-
- /** Guard for accepting drops in drop zone. Drag event is passed to this function as first parameter. This prop is necessary to be inlined to satisfy the linter */
- shouldAcceptDrop: PropTypes.func,
-
- /** Whether drag & drop should be disabled */
- disabled: PropTypes.bool,
+/**
+ * @param {Event} event – drag event
+ * @returns {Boolean}
+ */
+function shouldAcceptDrop(event) {
+ return _.some(event.dataTransfer.types, (type) => type === 'Files');
+}
- /** Rendered child component */
- children: PropTypes.node.isRequired,
-};
+function DragAndDrop({onDragEnter, onDragLeave, onDrop, dropZoneID, activeDropZoneID, children, isDisabled = false}) {
+ const isFocused = useIsFocused();
+ const {windowWidth, isSmallScreenWidth} = useWindowDimensions();
-const defaultProps = {
- onDragOver: () => {},
- shouldAcceptDrop: (e) => {
- if (e.dataTransfer.types) {
- for (let i = 0; i < e.dataTransfer.types.length; i++) {
- if (e.dataTransfer.types[i] === 'Files') {
- return true;
- }
- }
- }
- return false;
- },
- disabled: false,
-};
-
-function DragAndDrop(props) {
- 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
- */
- const dropZoneDragState = useRef(DRAG_LEAVE_EVENT);
- /**
- * @param {Object} event native Event
- */
- const dragOverHandler = (event) => {
- props.onDragOver(event);
- };
-
- const throttledDragOverHandler = _.throttle(dragOverHandler, 100);
-
- 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 {
- width: boundingClientRect.width,
- left: window.innerWidth <= variables.mobileResponsiveWidthBreakpoint ? 0 : boundingClientRect.left,
- right: window.innerWidth <= variables.mobileResponsiveWidthBreakpoint ? window.innerWidth : boundingClientRect.right,
- top: boundingClientRect.top,
- bottom: boundingClientRect.bottom,
- };
- }, []);
+ useEffect(() => {
+ dropZone.current = document.getElementById(dropZoneID);
+ }, [dropZoneID]);
+
+ useEffectOnPageLoad(
+ () =>
+ _.throttle(() => {
+ const boundingClientRect = dropZone.current.getBoundingClientRect();
+ dropZoneRect.current = {
+ width: boundingClientRect.width,
+ left: isSmallScreenWidth ? 0 : boundingClientRect.left,
+ right: isSmallScreenWidth ? windowWidth : boundingClientRect.right,
+ top: boundingClientRect.top,
+ bottom: boundingClientRect.bottom,
+ };
+ }, 100),
+ [windowWidth, isSmallScreenWidth],
+ );
- const dragNDropWindowResizeListener = () => {
- // Update bounding client rect on window resize
- dropZoneRect.current = calculateDropZoneClientReact();
- };
+ /*
+ * 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
+ */
+ const dropZoneDragState = useRef(DRAG_LEAVE_EVENT);
- const throttledDragNDropWindowResizeListener = _.throttle(dragNDropWindowResizeListener, 100);
+ // If this component is out of focus or disabled, reset the drag state back to the default
+ useEffect(() => {
+ if (isFocused && !isDisabled) {
+ return;
+ }
+ dropZoneDragState.current = DRAG_LEAVE_EVENT;
+ }, [isFocused, isDisabled]);
/**
+ * Handles all types of drag-N-drop events on the drop zone associated with composer
+ *
* @param {Object} event native Event
*/
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;
- 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 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;
+ if (!isFocused || isDisabled) {
+ return;
}
- },
- [props, throttledDragOverHandler],
- );
- /**
- * Handles all types of drag-N-drop events on the drop zone associated with composer
- *
- * @param {Object} event native Event
- */
- const dropZoneDragListener = useCallback(
- (event) => {
event.preventDefault();
- if (dropZone.current.contains(event.target) && props.shouldAcceptDrop(event)) {
- dropZoneDragHandler(event);
+ if (dropZone.current.contains(event.target) && shouldAcceptDrop(event)) {
+ switch (event.type) {
+ 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;
+ onDragEnter(event);
+ }
+ 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') === activeDropZoneID && !event.relatedTarget)
+ ) {
+ dropZoneDragState.current = DRAG_LEAVE_EVENT;
+ onDragLeave(event);
+ }
+ }
+ break;
+ case DROP_EVENT:
+ dropZoneDragState.current = DRAG_LEAVE_EVENT;
+ onDrop(event);
+ break;
+ default:
+ break;
+ }
} else {
// eslint-disable-next-line no-param-reassign
event.dataTransfer.dropEffect = NONE_DROP_EFFECT;
}
},
- [props, dropZoneDragHandler],
+ [isFocused, isDisabled, onDragEnter, onDragLeave, activeDropZoneID, onDrop],
);
- 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(() => {
- if (props.disabled) {
- return;
- }
- addEventListeners();
-
- return removeEventListeners;
- // eslint-disable-next-line react-hooks/exhaustive-deps
- }, []);
-
- useEffect(() => {
- if (props.isFocused === prevIsFocused && props.disabled === prevIsDisabled) {
- return;
- }
- if (!props.isFocused || props.disabled) {
- removeEventListeners();
- } else {
- addEventListeners();
- }
- }, [props.disabled, props.isFocused, prevIsDisabled, prevIsFocused, addEventListeners, removeEventListeners]);
+ document.addEventListener(DRAG_ENTER_EVENT, dropZoneDragHandler);
+ document.addEventListener(DRAG_LEAVE_EVENT, dropZoneDragHandler);
+ document.addEventListener(DROP_EVENT, dropZoneDragHandler);
+ return () => {
+ document.removeEventListener(DRAG_ENTER_EVENT, dropZoneDragHandler);
+ document.removeEventListener(DRAG_ENTER_EVENT, dropZoneDragHandler);
+ document.removeEventListener(DRAG_LEAVE_EVENT, dropZoneDragHandler);
+ document.removeEventListener(DROP_EVENT, dropZoneDragHandler);
+ };
+ }, [dropZoneDragHandler]);
- return props.children;
+ return children;
}
-DragAndDrop.propTypes = propTypes;
-DragAndDrop.defaultProps = defaultProps;
+DragAndDrop.propTypes = DragAndDropPropTypes;
-export default withNavigationFocus(DragAndDrop);
+export default DragAndDrop;
diff --git a/src/components/DragAndDrop/index.native.js b/src/components/DragAndDrop/index.native.js
index 4c00e56130..8d678752e7 100644
--- a/src/components/DragAndDrop/index.native.js
+++ b/src/components/DragAndDrop/index.native.js
@@ -1,5 +1,8 @@
-const DragAndDrop = (props) => props.children;
+import DragAndDropPropTypes from './dragAndDropPropTypes';
+const DragAndDrop = ({children}) => children;
+
+DragAndDrop.propTypes = DragAndDropPropTypes;
DragAndDrop.displayName = 'DragAndDrop';
export default DragAndDrop;
diff --git a/src/hooks/useEffectOnPageLoad.js b/src/hooks/useEffectOnPageLoad.js
new file mode 100644
index 0000000000..2f5a40217c
--- /dev/null
+++ b/src/hooks/useEffectOnPageLoad.js
@@ -0,0 +1,24 @@
+import {useEffect, useRef} from 'react';
+
+/**
+ * @param {Function} onPageLoad
+ * @param {Array} dependencies
+ */
+export default function useEffectOnPageLoad(onPageLoad, dependencies = []) {
+ const onPageLoadRef = useRef(onPageLoad);
+ onPageLoadRef.current = onPageLoad;
+
+ useEffect(() => {
+ function onPageLoadCallback() {
+ onPageLoadRef.current();
+ }
+
+ if (document.readyState === 'complete') {
+ onPageLoadCallback();
+ } else {
+ window.addEventListener('load', onPageLoadCallback);
+ return () => window.removeEventListener('load', onPageLoadCallback);
+ }
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, dependencies);
+}
diff --git a/src/pages/home/report/ReportActionCompose.js b/src/pages/home/report/ReportActionCompose.js
index 8155f09b2a..b1b8ad18dd 100644
--- a/src/pages/home/report/ReportActionCompose.js
+++ b/src/pages/home/report/ReportActionCompose.js
@@ -1112,8 +1112,8 @@ class ReportActionCompose extends React.Component {
</AttachmentPicker>
<View style={[styles.textInputComposeSpacing, styles.textInputComposeBorder]}>
<DragAndDrop
- dropZoneId={this.props.dragAndDropId}
- activeDropZoneId={CONST.REPORT.ACTIVE_DROP_NATIVE_ID + this.props.reportID}
+ dropZoneID={this.props.dragAndDropId}
+ activeDropZoneID={CONST.REPORT.ACTIVE_DROP_NATIVE_ID + this.props.reportID}
onDragEnter={() => {
this.setState({isDraggingOver: true});
}}
@@ -1133,7 +1133,7 @@ class ReportActionCompose extends React.Component {
this.setState({isDraggingOver: false});
}}
- disabled={this.props.disabled}
+ isDisabled={this.props.disabled}
>
<Composer
checkComposerVisibility={() => this.checkComposerVisibility()}
diff --git a/src/pages/home/report/ReportDropUI.js b/src/pages/home/report/ReportDropUI.js
index f59d0a3113..6e0e6ef68c 100644
--- a/src/pages/home/report/ReportDropUI.js
+++ b/src/pages/home/report/ReportDropUI.js
@@ -16,7 +16,7 @@ function ReportDropUI(props) {
return (
<DropZone
dropZoneViewHolderName={CONST.REPORT.DROP_HOST_NAME}
- dropZoneId={CONST.REPORT.ACTIVE_DROP_NATIVE_ID}
+ dropZoneID={CONST.REPORT.ACTIVE_DROP_NATIVE_ID}
>
<View style={styles.mb3}>
<Icon
diff --git a/src/stories/DragAndDrop.stories.js b/src/stories/DragAndDrop.stories.js
index 0398e5af3a..ece5394f49 100644
--- a/src/stories/DragAndDrop.stories.js
+++ b/src/stories/DragAndDrop.stories.js
@@ -27,8 +27,8 @@ function Default() {
{/* DragAndDrop does not need to render drop area as children since it is connected to it via id, which gives us flexibility to bring DragAndDrop where your
draggingOver state is located */}
<DragAndDrop
- dropZoneId="dropId"
- activeDropZoneId="activeDropZoneId"
+ dropZoneID="dropId"
+ activeDropZoneID="activeDropZoneID"
onDragEnter={() => {
setDraggingOver(true);
}}
@@ -75,7 +75,7 @@ function Default() {
{draggingOver && (
<DropZone
dropZoneViewHolderName="portalHost"
- dropZoneId="activeDropZoneId"
+ dropZoneID="activeDropZoneID"
/>
)}
</PortalProvider>
and looking at the new state of the component, it looks like this:
import {useCallback, useEffect, useRef} from 'react';
import _ from 'underscore';
import {useIsFocused} from '@react-navigation/native';
import DragAndDropPropTypes from './dragAndDropPropTypes';
import useWindowDimensions from '../../hooks/useWindowDimensions';
import useEffectOnPageLoad from '../../hooks/useEffectOnPageLoad';
const COPY_DROP_EFFECT = 'copy';
const NONE_DROP_EFFECT = 'none';
const DRAG_ENTER_EVENT = 'dragenter';
const DRAG_LEAVE_EVENT = 'dragleave';
const DROP_EVENT = 'drop';
/**
* @param {Event} event – drag event
* @returns {Boolean}
*/
function shouldAcceptDrop(event) {
return _.some(event.dataTransfer.types, (type) => type === 'Files');
}
function DragAndDrop({onDragEnter, onDragLeave, onDrop, dropZoneID, activeDropZoneID, children, isDisabled = false}) {
const isFocused = useIsFocused();
const {windowWidth, isSmallScreenWidth} = useWindowDimensions();
const dropZone = useRef(null);
const dropZoneRect = useRef(null);
useEffect(() => {
dropZone.current = document.getElementById(dropZoneID);
}, [dropZoneID]);
useEffectOnPageLoad(
() =>
_.throttle(() => {
const boundingClientRect = dropZone.current.getBoundingClientRect();
dropZoneRect.current = {
width: boundingClientRect.width,
left: isSmallScreenWidth ? 0 : boundingClientRect.left,
right: isSmallScreenWidth ? windowWidth : boundingClientRect.right,
top: boundingClientRect.top,
bottom: boundingClientRect.bottom,
};
}, 100),
[windowWidth, isSmallScreenWidth],
);
/*
* 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
*/
const dropZoneDragState = useRef(DRAG_LEAVE_EVENT);
// If this component is out of focus or disabled, reset the drag state back to the default
useEffect(() => {
if (isFocused && !isDisabled) {
return;
}
dropZoneDragState.current = DRAG_LEAVE_EVENT;
}, [isFocused, isDisabled]);
/**
* Handles all types of drag-N-drop events on the drop zone associated with composer
*
* @param {Object} event native Event
*/
const dropZoneDragHandler = useCallback(
(event) => {
if (!isFocused || isDisabled) {
return;
}
event.preventDefault();
if (dropZone.current.contains(event.target) && shouldAcceptDrop(event)) {
switch (event.type) {
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;
onDragEnter(event);
}
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') === activeDropZoneID && !event.relatedTarget)
) {
dropZoneDragState.current = DRAG_LEAVE_EVENT;
onDragLeave(event);
}
}
break;
case DROP_EVENT:
dropZoneDragState.current = DRAG_LEAVE_EVENT;
onDrop(event);
break;
default:
break;
}
} else {
// eslint-disable-next-line no-param-reassign
event.dataTransfer.dropEffect = NONE_DROP_EFFECT;
}
},
[isFocused, isDisabled, onDragEnter, onDragLeave, activeDropZoneID, onDrop],
);
useEffect(() => {
document.addEventListener(DRAG_ENTER_EVENT, dropZoneDragHandler);
document.addEventListener(DRAG_LEAVE_EVENT, dropZoneDragHandler);
document.addEventListener(DROP_EVENT, dropZoneDragHandler);
return () => {
document.removeEventListener(DRAG_ENTER_EVENT, dropZoneDragHandler);
document.removeEventListener(DRAG_ENTER_EVENT, dropZoneDragHandler);
document.removeEventListener(DRAG_LEAVE_EVENT, dropZoneDragHandler);
document.removeEventListener(DROP_EVENT, dropZoneDragHandler);
};
}, [dropZoneDragHandler]);
return children;
}
DragAndDrop.propTypes = DragAndDropPropTypes;
export default DragAndDrop;@AndrewGable discovered in #23046 that it was necessary to measure the dropZoneRect after the page loaded, so that's why I created that useEffectOnPageLoad hook.
| this.dropZoneDragListener = this.dropZoneDragListener.bind(this); | ||
|
|
||
| /* | ||
| function DragAndDrop(props) { |
There was a problem hiding this comment.
Please destructure props here
| dragOverHandler(event) { | ||
| this.props.onDragOver(event); | ||
| } | ||
| const dragOverHandler = (event) => { |
There was a problem hiding this comment.
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]);
...
...
}| const DRAG_LEAVE_EVENT = 'dragleave'; | ||
| const DROP_EVENT = 'drop'; | ||
| const RESIZE_EVENT = 'resize'; | ||
|
|
||
| const propTypes = { | ||
| ...DragAndDropPropTypes, |
There was a problem hiding this comment.
The onDragOver prop is included in DragAndDropProps so is duplicated on line 16. Let's remove it please.
There was a problem hiding this comment.
Actually, onDragOver is unused by any code consuming this component, so we should just remove it.
| const DRAG_LEAVE_EVENT = 'dragleave'; | ||
| const DROP_EVENT = 'drop'; | ||
| const RESIZE_EVENT = 'resize'; | ||
|
|
||
| const propTypes = { |
There was a problem hiding this comment.
Also:
- the
disabledprop should be renamed toisDisabled dropZoneIdshould be renamed todropZoneIDactiveDropZoneIdshould be renamed toactiveDropZoneID
|
|
||
| if (this.dropZone.contains(event.target) && this.props.shouldAcceptDrop(event)) { | ||
| this.dropZoneDragHandler(event); | ||
| if (dropZone.current.contains(event.target) && props.shouldAcceptDrop(event)) { |
There was a problem hiding this comment.
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.
| window.removeEventListener(RESIZE_EVENT, throttledDragNDropWindowResizeListener); | ||
| }, [dropZoneDragListener, throttledDragNDropWindowResizeListener]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| const DRAG_LEAVE_EVENT = 'dragleave'; | ||
| const DROP_EVENT = 'drop'; | ||
| const RESIZE_EVENT = 'resize'; | ||
|
|
||
| const propTypes = { | ||
| ...DragAndDropPropTypes, |
There was a problem hiding this comment.
Also, I noticed that DragAndDropPropTypes are not used outside this file...
56 tasks
|
Closing this as I was informed that @jczekalski is going to be working on other projects. Thanks for your work on this so far @jczekalski. I'm going to create a new PR based on this one incorporating my above feedback since this is HOLDing another PR that's a roadmap priority. @allroundexperts I'll request your review on my PR when it's ready |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Fixed Issues
$ #16142
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
Not sure if drag and drop to chat feature can be tested on this platform.Mobile Web - Safari
Not sure if drag and drop to chat feature can be tested on this platform.Desktop
electron.mov
iOS
Not sure if drag and drop to chat feature can be tested on this platform.Android
Not sure if drag and drop to chat feature can be tested on this platform.