rodrigodagostino / svelte-sortable-list

A package to create accessible reorderable lists in Svelte
https://svelte-sortable-list.netlify.app
MIT License
15 stars 3 forks source link

This package is awesome!! №2 #8

Open interk0t opened 3 weeks ago

interk0t commented 3 weeks ago

I really liked your library. It includes the key functionality that should be for a sorting list. Many similar libraries either do not support Svelte or do not have the necessary functionality for such a library. The key aspects that are positive for this library that can be highlighted: Functionality:

What I would like to see in the next versions of this library, and what functionality is missing (my opinion):

Methods:

Props:

Issues: I tried to create 1000 sortable elements and with such a number, the fps in the browser significantly decreased and it became difficult to move the element, since the animations lagged, is it possible to make some optimizations in this regard. I understand that the scenario in which the user will have 1000 elements is unlikely, and even if the developer does not use virtualization, but if there is an opportunity to optimize, it would be great.

I want to say that in my opinion, having tried almost all the libraries dedicated to sorting lists, this library solves many problems and it has a huge potential to become the best among the rest, this will be a good contribution to the Svelte community!

rodrigodagostino commented 3 weeks ago

Hi, @interk0t! :) Thank you very much for reaching out, and thank you for taking the time to provide all that feedback.

First of all, I gotta tell you I’m very glad you noticed the smooth animations and element interactions. I put a considerable amount of effort to have them work the way they do, specially in mobile devices, and I’m very pleased to know this can also be perceived by other people :)

You also laid down a very interesting roadmap for me, so let’s go point by point:

Support for multiple lists/droppables

These past few days I’ve been trying out a few different possibilities that came to mind, and for the moment it seems only two of them might be possible:

To be honest, I don’t even know if the second option will actually be fully plausible. What do you think? Would it be a good trade-off to add a mandatory new component into the mix in order to support multiple lists? @elron feel free to chime in too :) The more I think about it, the more I see no way out of having a mandatory <SortableContext>.

SortableList.enable() and SortableList.disable() methods

How could a feature like that be added considering the architecture of Svelte components? Is there an example that you could provide? I’m very interested in this feature.

Support scrolling while dragging

To be honest, I don’t even know what is necessary for this to be possible, but I will look into it, it sounds like a good addition ;)

Events

It should be fairly simple to add events like on:init, on:dragstart, and on:drop. I will also look into it. I was thinking that I could even replace the current on:sort and on:remove for something more broad like the list you suggested.

Props

isSortable and isSortableBetweenZones sound like good additions, so that’s also something I will try to implement.

Virtual lists

At some point I considered virtualizing the lists when needed, but it felt like something that wasn’t really necessary for this little package. I mean, I was thinking that most people making use of it won’t be creating lists of 500 items or more. Accessibility is one of my priorities here, and virtual lists are not that friendly when it comes to a11y. But I would like to at least provide the support needed for developers who want to use virtual lists. I’m not sure what would be needed from my side to lay the grounds for that, but I will look into it :)

Optimization

I usually try to optimize as much as I can of the code I write. That doesn’t mean that there are no improvements left to be done, but I’ve definitely covered as much as my knowledge in programming allowed me to :P

I gotta thank you again, honestly! Your feedback was very inspiring and has fueled my hunger for this package even more. It’s clear to see that you have considerable experience in this matter, and I’m grateful for the time you spent setting some guidelines for me. I will make good use of them :)

elron commented 3 weeks ago

SortableContext

Having <SortableContext> as a mandatory is fine as long as it works :) It's not a big deal.

But if you want even cleaner, maybe you can create a new svelte component: <SingleSortableList> that actually invokes both <SortableContext><SortableList /></SortableContext>.

but anyway, I wouldn't mind adding <SortableContext> even for single list.

Support scrolling while dragging

I agree that support scrolling while dragging is very important, as I have a very large item set (500~ items that go in about 20-30 different lists).

I know it sounds a lot but actually this is how big my lists are. (I use react-beautiful-dnd to sort course content and lessons, so each list is a chapter and each lesson is an item).

interk0t commented 1 week ago

From a compactness point of view, it would be good not to use context in cases where there is only one list, it would be super convenient.

SortableList.enable() and SortableList.disable() methods

To be honest, I have no experience writing libraries, but usually such methods are added to libraries that provide functionality at the class level, I don’t know how your library works, but maybe it was possible to pass this value as a prop, and going so that this method/prop completely disables the library functionality on a specific element (removing style classes, etc.)

Support scrolling while dragging

A very useful thing, and it is very necessary when the list of elements is larger than the height or width of the screen

Virtual lists It would also be very useful, but this is the last one, as it seems to me

Well, how can I say, I have little experience in programming, but I really needed such a library for svelte, and none of the existing ones met the requirements, although the requirements were quite logical and necessary for such a package, your library ALREADY solves most of the problems, and in my opinion the best option of all existing ones, and if its improve, partially or completely based on my suggestions, that would be great

elron commented 1 week ago

Inspired by @interk0t last message:

From a compactness point of view, it would be good not to use context in cases where there is only one list, it would be super convenient.

Just throwing an idea for a better name than <SortableContext>, if we want it to support single lists without content wrapped:

<SortableList> - main (can work stand alone - all event dispatchers goes here) <SortableSublist> (inside SortableList, but not mandatory)

I imagine that you can have unlimited nested sublists (<SortableSublist> inside <SortableSublist>)

If we need to specify If there are nested or not, Perhaps we can do it with a prop:

<SortableList single> or <SortableList hasSublists>

That way inside the wrapper component we can do:

{#if hasSublists}
    <slot />
{:else}
    <SortableSublist>
{/if}