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

[5.x] Ability to drag/drop sets between replicators #10361

Open
wants to merge 18 commits into
base: 5.x
Choose a base branch
from

Conversation

jacksleight
Copy link
Contributor

@jacksleight jacksleight commented Jun 24, 2024

This PR adds the ability to drag and drop sets between different replicators:

CleanShot.2024-07-04.at.10.57.24.mp4

Notes

  • I had to change the way the draggable instances are initialised so that the same instance can be shared between multiple lists, but this shouldn't impact any non-replicator lists.
  • The sortable list component accepts a new group prop that tells it which instance to share, and a groupValidator function that's used to decide whether the item is valid in the target container.
  • Replicators use a list of set config hashes to decide whether the set being dragged is compatible with the target replicator.
  • I had to give the set container a minimum height so that there was still a drop area available when the replicator is empty. This may need some adjustment.
  • If group is set the input event emitted from the sortable list component has different data, allowing the parent component to decide how to handle the changes.
  • The sortable list move helper function has been moved to globals and renamed/split up so that functionality can be used elsewhere.
  • I’ve added not-allowed cursors when dragging sets over incompatible replicators. The only way I could make that work well with nested replicators was by using the new-ish CSS scope feature. That has decent but not full browser support right now.
  • Annoyingly there’s a bug in Shopify draggable that causes an uncaught error when dragging a set into an empty replicator. It doesn’t break anything, but it shows up in console. A fix was submitted but it doesn’t look like anything is happening with it unfortunately.

Other Changes

I’ve made a couple of changes that are not strictly drag-and-drop related, but were required to implement this nicely:

  • The hashes and blink keys implemented in [5.x] Add cache for nested imported fieldsets #10280 have been slightly refactored. This PR requires hashes for each set type, and that PR needs the same thing. Rather than generating those twice both features now use the same list of set hashes.
  • I’ve also tweaked some of the other hashing to avoid having to hash the same config multiple times (which adds unnecessary overhead). Each field instance now only hashes its config once, which is implemented via a new configHash() method on the Field class (only called when needed).
  • I’ve updated a test that was creating a Bard object without setting a field. This meant that the field could be null when generating the config hash, resulting in blink keys that are pretty meaningless. Rather than allowing null I thought it made more sense to adjust the test.

@aerni
Copy link
Contributor

aerni commented Jun 28, 2024

Love this idea! From a UX perspective, it might be a good idea to add some sort of indicator if a set can be placed in another replicator. Like in your demo above, you can't move an image set into a text-only replicator. But this is not apparent to the user. So that's quite confusing.

@jacksleight jacksleight marked this pull request as ready for review July 4, 2024 10:47
@jacksleight
Copy link
Contributor Author

jacksleight commented Jul 4, 2024

This is ready for review!

@aerni Following your suggestion I added the not-allowed cursors, but I think you probably meant something more, like highlighting the compatible replicators? I tried to come up with a nice UI for that but couldn't figure out anything that a) was obvious but not too obtrusive and b) worked well with nested replicators. I’ll wait for Jack and the team to review and see if they have any suggestions.

@aerni
Copy link
Contributor

aerni commented Jul 4, 2024

The not-allowed cursor is already a good hint. But yeah, would probably reduce the opacity on the replicators or something like that. Let's see what Jack thinks.

@jacksleight
Copy link
Contributor Author

would probably reduce the opacity on the replicators

This would be slick, and I tried it, but if you're dragging between nested replicators and the parent isn't compatible that will fade out, fading out the nested ones as well.

@daun
Copy link
Contributor

daun commented Dec 6, 2024

@jacksleight This looks fantastic! I've been dreaming about this exact feature, just for asset fields. Do you see some sort of overlap between such a feature and the additions in your PR, in terms of code re-use? Or would this be better off in a separate implementation over in asset land?

@jacksleight
Copy link
Contributor Author

jacksleight commented Dec 6, 2024

@daun I think you could totally make use of some of the additions here. Only about a half of this is replicator specific, the other half is general changes to the global sortable list component that allows:

  1. Putting multiple lists into a group, so items can be dragged between them.
  2. Changes to the events that are fired if a group is set, so you can handle move-out/in and validate drop targets.

@daun
Copy link
Contributor

daun commented Dec 6, 2024

@jacksleight Nice! I'll wait until this one gets merged then, and see what I can re-use if/when I give it a try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants