Made react-easy-sort work out-of-the-box with items of variable size

ValentinH / react-easy-sort

A React component to sort items in lists or grids

https://ValentinH.github.io/react-easy-sort

MIT License

160 stars 25 forks source link

Made react-easy-sort work out-of-the-box with items of variable size #26

Closed DCzajkowski closed 1 year ago

DCzajkowski commented 2 years ago

Closes #25

For reviewer's information: I haven't ran Prettier on the entire file of src/index.tsx, because that triggered some unrelated changes. I suggest maintainers to run Prettier on src/index.tsx after merge.

https://user-images.githubusercontent.com/4501047/150538621-7bd0173f-2da0-48b6-a48a-6ae71ae9abad.mov

DCzajkowski commented 2 years ago

I have just noticed this change breaks the "Interactive avatars" demo. Indeed, in grid layouts this solution won't work.

Probably an additional prop would be required or some smart layout recognition. Can someone take it from here?

DCzajkowski commented 2 years ago

I have just noticed this change breaks the "Interactive avatars" demo. Indeed, in grid layouts this solution won't work.

Probably an additional prop would be required or some smart layout recognition. Can someone take it from here?

@ValentinH what if we added a new prop like variable: boolean, that would adapt height? I am fine with scraping this PR as well, if you feel like so :)

ValentinH commented 2 years ago

@DCzajkowski I'm sorry for not being active on this repo at the moment, I've a very limited availability. I also need to recover my admin rights after leaving the Ricardo organisation to be able to maintain this repo.

I'll try to come back to this asap.

Regarding the changes in this PR: I think this feature is a nice one but I'd like to have a generic implementation that support both static and variable sizes, without any new option. It should be doable as far as I understand.

DCzajkowski commented 2 years ago

I've a very limited availability.

Don't worry, there is not rush at all! We are currently using the forked version, so getting it merged in the upstream is just a nice-to-have.

I'd like to have a generic implementation that support both static and variable sizes, without any new option. It should be doable as far as I understand.

It's extremely tricky with grids — not even on the implementation level, but the conceptual one. There are a lot of edge cases like moving larger element in place of a smaller one and vice versa. Then there is adapting elements' sizes to the column sizes. On top of that, there are grids that flow left-to-right and those that flow top-to-bottom. Doing it to work for all will be hard 😅

ValentinH commented 2 years ago

You are right, grids will be really hard to support. Let's not worry about them now but just make sure we don't break them. Grid layout was actually the reason why I created this library i n the first place 😅

ValentinH commented 2 years ago

I just have a look again at this PR in order to merge it. However, I noticed 2 small issues:

on the horizontal demo, if I drag A after B and then try to move it back without releasing it, I can't 🤔
not a bug but I think the trigger to change the placeholder position should also be adjusted based on the hovered item size. In the following gif, I show a case where I think the items are swap too early when dragging. I think we should only swap them when hovering near the center. I'm not sure but we would try this to see if it feels better:

DCzajkowski commented 1 year ago

I am closing this as it doesn't seem to be a great solution and there is no interest in pushing this further.