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.

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

chore(ui): migrate dnd library #7288

Merged
merged 39 commits into from
Nov 7, 2024
Merged

Conversation

psychedelicious
Copy link
Collaborator

Summary

This PR migrates the application from dnd-kit to pragmatic-drag-and-drop.

Why

dnd-kit is a pure JS library, wholly divorced from browsers' native dnd. Dragging is accomplished by creating drag preview elements and moving them with JS.

Using JS has some inherent issues:

  • It's not particularly performant - you have to listen to mouse move events to move things around.
  • It cannot handle external drag events (e.g. from the OS). You have to implement those separately, which is painful enough that I haven't tried. Doable, yes, but fun, definitely not.

The library has some other issues:

  • Its use of react context results in re-rendering huge swathes of the application whenever drag state changes.
  • It is annoying to make components draggable and droppable.
  • It is difficult and painful to make components draggable and droppable and sortable. Maybe impossible? Dunno.
  • It is quasi-maintained. There's been a rewrite in progress for months, with no ETA.

pragmatic-drag-and-drop is a newcomer, created by Atlassian and powering their products. It uses native browser APIs for core functionality instead of JS.

Its implementation resolves all of the issues with dnd-kit:

  • Super fast - it lets the browser do the heavy lifting outside JS-land.
  • Provides a unified API for element and external drag events.
  • It is designed in such a way that it is hard to cause unnecessary re-renders.
  • It is trivial to attach any and all dnd behaviours to elements.
  • It is actively maintained as a core part of Atlassian's massive product suite. It is open-source, but not open-contribution, but Atlassian has actually fixed browser bugs during development of the library. It is safe to built on this library.

Pdnd is a vanilla JS library, but it works very well with react and there are many react examples.

Changes

This PR migrates the library just a little bit past feature parity - it makes layers in Canvas sortable via dnd. Otherwise, the functionality in this PR is nearly identical*.

As part of the migration, I reworked a lot of logic around gallery & dnd-capable image rendering, making the components a lot simpler and providing a noticeable perf boost.

I also removed the type SerializableObject, switching to JsonObject from type-fest. Probably should have done it separately but it made some type stuff easier.

Generalized image actions

Many image actions were implemented 2 or 3 times.

For example, "create raster layer from image" was implemented in at least 3 places:

  • In a dnd RTK middleware listener
  • In an image uploaded RTK middleware listener
  • In a hook used in the image context menu

To clean this up, these actions have been consolidated. Also, dnd is not handled in redux. It's in a pdnd "monitor" function.

*Nearly identical

Workflow field styling

There was an awkward border when mousing over fields, indicating they are related or for the selected node. This is now a small dot.

Multi-select dnd

  • Old: if you have multiple images selected but start a drag from an unselected image, it is treated as if you dragged the whole selection.
  • New: if you have multiple images selected but start a drag from an unselected image, it is treated as if you dragged the whole selection.

Dragging images into the app from the OS

Previously, we used react-dropzone for the full-page external dnd. This library lets you do validation on drag payloads as the drag starts. For example, we could check if the user is dragging only images and show an error before they try to drop.

Unfortunately, this is a nonstandard API - you shouldn't be able to access the file info before the drop for security reason. It appears Chrome and FF ignore this restriction and let you access file info, but Safari does not. In fact, dnd from external sources into the app has never worked correctly with Safari.

pragmatic-drag-and-drop takes the position that if it cannot provide the same API across all browsers, it won't provide the API at all. As a result, when you drag files in from the OS, we cannot validate them until you drop.

Related Issues / Discussions

Offline discussion.

QA Instructions

  • Drag images in from outside the app
  • Drag images around in the app
  • Drag layers to sort them
  • Drag workflow linear fields to sort them (edit mode only)
  • When a drop target is within a scrollable container (e.g. workflow linear fields or boards list), it should autoscroll as you drag towards the edges of the container
  • Make sure all the upload buttons still exist and work
  • Make sure various drop targets still link to the same actions

Merge Plan

Nothing special.

Checklist

  • The PR has a short but descriptive title, suitable for a changelog

@github-actions github-actions bot added frontend-deps PRs that change frontend dependencies frontend PRs that change frontend files labels Nov 7, 2024
@psychedelicious psychedelicious enabled auto-merge (rebase) November 7, 2024 20:37
We don't need a "dnd" image system. We need a "image action" system. We need to execute specific flows with images from various "origins":
- internal dnd e.g. from gallery
- external dnd e.g. user drags an image file into the browser
- direct file upload e.g. user clicks an upload button
- some other internal app button e.g. a context menu

The actions are now generalized to better support these various use-cases.
@psychedelicious psychedelicious merged commit c37251d into main Nov 7, 2024
14 checks passed
@psychedelicious psychedelicious deleted the psyche/chore/ui/migrate-dnd branch November 7, 2024 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend PRs that change frontend files frontend-deps PRs that change frontend dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants