DEP Replace react-dnd with dnd-kit #1527

GuySartorelli · 2024-12-17T23:01:43Z

This PR replaces the react-dnd with dnd-kit with like-for-like functionality, except:

Dragged file is the wrong size #1016 has been fixed - I had to alter the styling anyway because of the way dragging the GalleryItemDragLayer works now. The changes aren't fully compatible with CMS 5 so I won't be backporting the changes there, so #1016 needs to remain open.
When dragging an item on top of a droppable zone, the cursor doesn't update. The one used by react-dnd is actually coming from the browser itself with default HTML5 drag-and-drop functionality (see https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer/dropEffect). That cursor is not selectable with CSS as far as I can tell.
- We could use the no-drop cursor when dragging anywhere else, and only use grabbing for when we're over a valid drop target - but that wouldn't be consistent with cursors used in other drag-and-drop scenarios in the CMS. I'd recommend if we want to do that, it's handled in a wider-reaching card that standardises this approach.

Ran yarn upgrade to get silverstripe/eslint-config#29

Issue

Switch from react-dnd to dnd-kit .github#343

GuySartorelli · 2024-12-19T01:26:10Z

client/src/containers/Gallery/GalleryDND.js

Note that I didn't split converting this or other components to functional as its own PR or commit because the lifycycle functions needed to be removed anyway. Converting it with the lifecycle events in-tact and then removing them separately would have been more work.

GuySartorelli · 2024-12-19T01:26:59Z

client/src/containers/Gallery/GalleryDND.js

-      <div className={classnames(className, { 'gallery__main--dragging': this.state.dragging })}>
+  return (
+    <div className={classnames(className, { 'gallery__main--dragging': dragging })}>
+      <DndContext sensors={sensors} onDragStart={handleDragStart} onDragEnd={handleDragEnd} modifiers={[snapCenterToCursor]}>


I tried using built-in modifiers (including restrictToWindowEdges) to restrict where we can drag the files, but none of them worked how I'd expect them to, so I opted to go without. The end result is the same unbound dragging as the old approach so it's not worse than it was before, at least.

GuySartorelli · 2024-12-19T01:30:30Z

client/src/components/GalleryItem/GalleryItem.js

+    if (this.props.item.type === 'folder') {
+      thumbnailClassNames.push('gallery-item__thumbnail--folder');
+    }


Needed for styling the draglayer for folders

GuySartorelli · 2024-12-19T01:31:21Z

client/src/components/GalleryItem/GalleryItem.scss

-  background: $white url("../../images/icon_archive.png") center center no-repeat;
+  background: transparent url("../../images/icon_archive.png") center center no-repeat;


These changes were required to fix a problem where the background was being drawn over top of the border of the drag layer.
Note that I can't just use background-image because that seems to be overridden by something elseand results in no thumbnail being displayed

GuySartorelli · 2024-12-19T01:32:15Z

client/src/components/GalleryItem/GalleryItemDragLayer.js

-  getOffset() {
-    const {
-      offset,
-      dragged,
-    } = this.props;
-    return {
-      transform: offset && `translate(${offset.x + dragged.x}px, ${offset.y + dragged.y}px)`,
-    };
-  }


Offset stuff is handled by the snapCenterToCursor modifier now

GuySartorelli · 2024-12-19T01:33:11Z

client/src/components/GalleryItem/README.md

-<GalleryItem item={{id: 2}} selectable={true} />
+<GalleryItem item={{id: 2, exists: true, type: 'file', category: 'document'}} selectable={true} />
+<GalleryItem item={{id: 2, exists: true, type: 'folder', category: 'folder'}} selectable={true} />


While updating styling, I had to render out these directly so I could mess with css in the browser.
I found that just an ID wasn't enough, so I've updated for an example for a file and for a folder, for future reference.

GuySartorelli · 2024-12-19T01:33:51Z

client/src/components/GalleryItem/draggable.js

-    canDrag(props) {
-      return props.canDrag;
-    },
-    beginDrag(props) {
-      const { id } = props.item;
-      if (typeof props.onDrag === 'function') {
-        props.onDrag(true, id);
-      }
-      const selected = props.selectedFiles.concat([]);
-      if (!selected.includes(id)) {
-        selected.push(id);
-      }
-
-      return { selected, props };
-    },
-    endDrag(props) {
-      const { id } = props.item;
-      if (typeof props.onDrag === 'function') {
-        props.onDrag(false, id);
-      }
-    },


Handling what happens on drag/drop is done inside GalleryDND instead.

GuySartorelli · 2024-12-19T01:34:49Z

client/src/containers/Gallery/Gallery.js

@@ -952,7 +950,12 @@ class Gallery extends Component {
        ref={gallery => { this.gallery = gallery; }}
      >
        {this.renderTransitionBulkActions()}
-        <GalleryDND className={galleryClasses.join(' ')}>
+        <GalleryDND
+          onDragStartEnd={(dragging) => this.handleEnableDropzone(!dragging)}


Replaces handleDrag in ThumbnailView

GuySartorelli · 2024-12-19T21:04:28Z

tests/behat/src/FixtureContext.php

+    public function stepDragTheFileToTheFolder(string $fileName, string $folderName): void
+    {
+        $file = $this->getGalleryItem($fileName)->find('xpath', "/ancestor-or-self::div[contains(@class, 'gallery-item__draggable')]");
+        Assert::assertNotNull($file, "File named {$fileName} could not be found or isn't draggable");
+        $folder = $this->getGalleryItem($folderName)->find('xpath', "/ancestor-or-self::div[contains(@class, 'gallery-item__droppable')]");
+        Assert::assertNotNull($folder, "Folder named {$folderName} could not be found or isn't droppable");
+        $file->dragTo($folder);
+    }


I couldn't use the existing I drag the "some element" element to the "some other element" element method, because the only selectors available rely on fixed IDs which works locally but fails in CI.

emteknetnz

Couple of weird things happening when I tested locally in firefox, neither of which happen in CMS 5

After moving, the thumbnail animates back to its original location
There's a red toast error when attempting to move to the parent folder. Only happens with moving files, not moving folders.

https://www.youtube.com/watch?v=vnHESa15Q8g

emteknetnz · 2024-12-19T22:29:15Z

tests/behat/features/drag-and-drop.feature

@@ -0,0 +1,34 @@
+@javascript @assets @retry @job3 @gsat


Suggested change

@javascript @assets @retry @job3 @gsat

@javascript @assets @retry @job3

GuySartorelli · 2024-12-19T22:44:47Z

There's a server error when attempting to move to the parent folder. Only happens with moving files, not moving folders.

If the parent folder is the root, this is a known regression that was already linked in the issue description.

emteknetnz · 2024-12-19T22:56:57Z

Server error still happens when trying to move files to the parent folder in deeply nested folders

GuySartorelli · 2024-12-19T22:59:39Z

Can't reproduce. Please:

Compare against the 3.0 branch without this PR to rule out an existing regression
If it is an existing regression, open a new card about it
If it is not an existing regression, please provide server error message and stack trace - I may even ask for a copy of your DB and assets since as I said I can't reproduce.

GuySartorelli · 2024-12-19T23:15:00Z

After moving, the thumbnail animates back to its original location

Good spot. Fixed.

GuySartorelli marked this pull request as draft December 17, 2024 23:01

GuySartorelli mentioned this pull request Dec 17, 2024

Switch from react-dnd to dnd-kit silverstripe/.github#343

Open

GuySartorelli force-pushed the pulls/3.0/dnd-kit branch from 7ec8af0 to f14a00f Compare December 18, 2024 04:04

GuySartorelli mentioned this pull request Dec 19, 2024

ENH Allow more modern syntax including null-coallescing silverstripe/eslint-config#29

Merged

GuySartorelli force-pushed the pulls/3.0/dnd-kit branch 2 times, most recently from c26d42d to e54fea8 Compare December 19, 2024 01:29

GuySartorelli commented Dec 19, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/3.0/dnd-kit branch 5 times, most recently from 4728667 to 2c975bc Compare December 19, 2024 20:54

GuySartorelli marked this pull request as ready for review December 19, 2024 21:03

GuySartorelli commented Dec 19, 2024

View reviewed changes

emteknetnz requested changes Dec 19, 2024

View reviewed changes

DEP Replace react-dnd with dnd-kit

f9a4d8d

GuySartorelli force-pushed the pulls/3.0/dnd-kit branch from 2c975bc to f9a4d8d Compare December 19, 2024 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEP Replace react-dnd with dnd-kit #1527

DEP Replace react-dnd with dnd-kit #1527

GuySartorelli commented Dec 17, 2024 •

edited

Loading

GuySartorelli Dec 19, 2024

GuySartorelli Dec 19, 2024

GuySartorelli Dec 19, 2024

GuySartorelli Dec 19, 2024 •

edited

Loading

GuySartorelli Dec 19, 2024

GuySartorelli Dec 19, 2024

GuySartorelli Dec 19, 2024

GuySartorelli Dec 19, 2024

GuySartorelli Dec 19, 2024

emteknetnz left a comment •

edited

Loading

emteknetnz Dec 19, 2024

GuySartorelli Dec 19, 2024

GuySartorelli commented Dec 19, 2024

emteknetnz commented Dec 19, 2024

GuySartorelli commented Dec 19, 2024

GuySartorelli commented Dec 19, 2024

		background: $white url("../../images/icon_archive.png") center center no-repeat;
		background: transparent url("../../images/icon_archive.png") center center no-repeat;

	@javascript @assets @retry @job3 @gsat
	@javascript @assets @retry @job3

DEP Replace react-dnd with dnd-kit #1527

Are you sure you want to change the base?

DEP Replace react-dnd with dnd-kit #1527

Conversation

GuySartorelli commented Dec 17, 2024 • edited Loading

Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli commented Dec 19, 2024

emteknetnz commented Dec 19, 2024

GuySartorelli commented Dec 19, 2024

GuySartorelli commented Dec 19, 2024

GuySartorelli commented Dec 17, 2024 •

edited

Loading

GuySartorelli Dec 19, 2024 •

edited

Loading

emteknetnz left a comment •

edited

Loading