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

feature: repeater and builder item drag and drop ordering #2017

Merged
merged 14 commits into from Mar 29, 2022
Merged

feature: repeater and builder item drag and drop ordering #2017

merged 14 commits into from Mar 29, 2022

Conversation

jacksleight
Copy link
Contributor

@jacksleight jacksleight commented Mar 25, 2022

Drag and drop ordering for repeater items:

Screen.Recording.2022-03-25.at.11.37.39.mov

@danharrin
Copy link
Member

Thanks! Few points...

  • Test failing
  • I don't think we need extra button?
  • Does this work with nested repeaters?
  • How about the builder?

@danharrin danharrin added the enhancement New feature or request label Mar 25, 2022
@pxlrbt
Copy link
Member

pxlrbt commented Mar 25, 2022

Maybe we should integrate our own Livewire integration of SortableJS. It's less than 100 LOC in this case and we don't need to rely on 3rd party packages. There is not much activity on that repo currently.

UI wise I think I would prefer an area on the top-left/left side with a drag icon like this:
https://fontawesome.com/icons/grip-dots-vertical?s=solid

Unfortunately there's no drag icon in heroicons. Maybe we could merge two "dots-vertical" to a drag icon 😅

@pxlrbt
Copy link
Member

pxlrbt commented Mar 25, 2022

Btw tests are failing because you bumped min PHP version on 2.x 😅

@jacksleight jacksleight marked this pull request as draft March 25, 2022 13:57
@jacksleight
Copy link
Contributor Author

Yeah test error seems to be because of the composer requirements?

Works fine with nested repeaters.

Will change the drag handle as @pxlrbt suggested, and look at builder as well.

@jacksleight
Copy link
Contributor Author

jacksleight commented Mar 25, 2022

OK, made the suggested changes. I'm a bit unsure on how/where is best to import the custom Livewire directive, shout if it's wrong.

New drag handle style:

Screenshot 2022-03-25 at 15 17 12

Edit: Also the Livewire directive is basically a copy paste from the other package, but could be customised now it's in Filament.

@pxlrbt
Copy link
Member

pxlrbt commented Mar 25, 2022

Not sure how to improve this, but the drag handle feels a bit off 🙈

@jacksleight
Copy link
Contributor Author

Mmmm yeah, I do feel it's a bit close to the field label.

@pxlrbt
Copy link
Member

pxlrbt commented Mar 25, 2022

Edit: Also the Livewire directive is basically a copy paste from the other package, but could be customised now it's in Filament.

Wasn't the other implementation about 75 lines instead of 30? 🤔

@jacksleight
Copy link
Contributor Author

Wasn't the other implementation about 75 lines instead of 30? 🤔

Yeah, it included support for groups (for moving items between lists) which isn't needed so I didn't include it. Although that could potentially be an idea for a future feature, moving items between repeaters.

@jacksleight jacksleight changed the title feature: repeater item drag and drop ordering feature: repeater and builder item drag and drop ordering Mar 25, 2022
@pxlrbt
Copy link
Member

pxlrbt commented Mar 25, 2022

Can me move stuff between nested repeaters - as Dan mentioned - without groups?

@jacksleight
Copy link
Contributor Author

jacksleight commented Mar 25, 2022

Oh, no they're self-contained at the moment, you can't move something from one nested to repeater to a parent/child repeater at the moment... Do the regular up/down buttons support that?

I think moving things between repeaters (whether nested or elsewhere on the page) would need some kind of user configuration to define which repeaters are grouped with one another, as field configurations could be different.

@pxlrbt
Copy link
Member

pxlrbt commented Mar 25, 2022

Sure, you're right. Maybe that's out of scope for now.

@jacksleight
Copy link
Contributor Author

Yeah, certainly be nice to have, but maybe one for another day 🙂 .

@jacksleight
Copy link
Contributor Author

Now we're using a custom Livewire directive we shouldn't need the extra dispatch event methods anymore. I'll refactor that.

@wychoong
Copy link
Contributor

maybe have the drag handle at the top center, heroicon-s-dots-horizontal, like ipad multitask dots?

@panakour
Copy link
Contributor

panakour commented Mar 26, 2022

or maybe using a settings tooltip which let have more actions in the future without looking messy, something like they did in octobercms repeater:
Screenshot from 2022-03-26 13-23-09

@jacksleight
Copy link
Contributor Author

jacksleight commented Mar 26, 2022

I've knocked up a few drag handle variations, 1 is the current version.

I'd personally vote for 4, but I'm happy to implement whichever people think is best, or something else entirely. Let me know your thoughts.

Screenshot 2022-03-26 at 16 04 22
Screenshot 2022-03-26 at 16 03 45
Screenshot 2022-03-26 at 16 06 47
Screenshot 2022-03-26 at 16 07 41

@panakour
Copy link
Contributor

I also prefer the 4

@martinmildner
Copy link
Contributor

What about an up and down arrow?
Screenshot-2022-03-27 at 14 25 53@2x

@pxlrbt
Copy link
Member

pxlrbt commented Mar 27, 2022

What about an up and down arrow? Screenshot-2022-03-27 at 14 25 53@2x

Check out the video in the initial PR comment ;)

@danharrin
Copy link
Member

danharrin commented Mar 27, 2022

I think 3 and 4 are my favourites, but we can get rid the old up / down buttons maybe? What do you think?

@wychoong
Copy link
Contributor

I feel 4 is crowded but of course I’m biased towards 3 😆
If it’s not too hard to maintain, maybe ->showDragHandle(left/right/center) and ->hideSortButtons()

@pxlrbt
Copy link
Member

pxlrbt commented Mar 27, 2022

I guess with Drag & Drop we could remove the Up/Down arrow and get some space.

What about something like this? Inspired by other block editors and the October CMS solution. It takes up some more space but also gives a bigger dragging handle (whole grey background area)

  1. Left

Bildschirmfoto 2022-03-27 um 18 11 06

  1. Right

Bildschirmfoto 2022-03-27 um 17 19 42

  1. Top

Bildschirmfoto 2022-03-27 um 17 30 20

@martinmildner
Copy link
Contributor

+1 for 5 (aligned top left)

@jacksleight
Copy link
Contributor Author

jacksleight commented Mar 28, 2022

I really like @panakour and @pxlrbt’s ideas of having a larger handle that could include the other actions as well. And possibly making this configurable as @wychoong suggested.

But that probably needs more discussion, and I think it’d be good to get the core drag and drop functionality in place, so I’m gonna focus on that for this PR.

I think 3 and 4 are my favourites, but we can get rid the old up / down buttons maybe? What do you think?

I’ve implemented 4 for now. That’s the simplest solution with the smallest UI impact.

I’ve removed the up/down buttons but added arrow up/down key listeners to the drag handle, so it’s still keyboard accessible. Livewire seems to get glitchy if you add/remove these listeners on changes, so instead of toggling them in the view like the old buttons were I've added a check to the array_move_after and array_move_before functions.

@jacksleight jacksleight marked this pull request as ready for review March 28, 2022 08:53
jacksleight and others added 4 commits March 28, 2022 12:12
Co-authored-by: Dan Harrin <git@danharrin.com>
Co-authored-by: Dan Harrin <git@danharrin.com>
Co-authored-by: Dan Harrin <git@danharrin.com>
Co-authored-by: Dan Harrin <git@danharrin.com>
@a21ns1g4ts
Copy link
Contributor

I think 3 and 4 are my favourites, but we can get rid the old up / down buttons maybe? What do you think?

Yep. we can keep all clean

@danharrin danharrin merged commit f8254df into filamentphp:2.x Mar 29, 2022
@danharrin
Copy link
Member

Thank you very much! 🍻

@danharrin
Copy link
Member

Hey @jacksleight, after multiple bug reports it seems that this breaks sorting for nested repeaters. I've opened a PR which removes this feature here - #2156.

I really don't want to merge it, but I've spent a while trying to find a solution and I cannot. It feels like Livewire is conflicting with the sortable package, which causes the moved items to appear above the parent repeater (DOM-diffing issue?). @ryangjchandler or @jacksleight if you're able to look into this today or tomorrow, that would be great. But if you don't have any time then that's absolutely fine, but I will need to merge #2156, probably tomorrow afternoon.

@danharrin
Copy link
Member

To replicate the issue:

Repeater::make('options')
    ->schema([
        Repeater::make('values')
            ->schema([
                TextInput::make('value'),
            ])
    ]),

Add two items to the values repeater and then swap their positions.

@danharrin
Copy link
Member

Thanks @ryangjchandler, fixed by #2164.

@jacksleight
Copy link
Contributor Author

@danharrin Hey, sorry I've been away so didn't catch this, wasn't ignoring it 😄 . Glad you found a fix!

@danharrin
Copy link
Member

No worries mate, all sorted! Hope you had a good break

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

Successfully merging this pull request may close these issues.

None yet

8 participants