Skip to content
This repository has been archived by the owner on Feb 18, 2022. It is now read-only.

Sorting callback // use optional order attribute to sort the children #7

Open
Harti opened this issue Jul 2, 2015 · 15 comments
Open

Comments

@Harti
Copy link

Harti commented Jul 2, 2015

First of all, thanks for creating this! I've been desperately waiting for Ryan Florence to update his MagicMove library but it just wouldn't happen. So I'm really glad you stepped in and did some refactoring while at it.

I've got a pesky issue with your lib though: The children sorting. From both your example and the sources I assume that the ordering of child elements within the Shuffle container will be made by "greater-than" comparisons using the component's key attribute.
This seems to produce issues as React appears to use the key attribute as a unique identifier for a component.

Let me elaborate. Imagine the following racing situation UI:

[driverID: 3, pos: 3] <--- 1.8 s ---> [driverID: 2, pos: 2] <--- 0.7 s ---> [driverID: 1, pos: 1]

Since the drivers don't change in this scenario their IDs are used as the component's key attribute. React-shuffle would reorder my list and mess the position order up (3 being greater than 2, 2 being greater than 1).
As a workaround I use key={1000-pos} to get the right order. (we'll revisit this later)

I want to animate this inline-block list whenever any driver overtakes another driver, resulting in a position change. Like so:

[driverID: 3, pos: 3] <--- 2.1 s ---> [driverID: **1**, pos: 2] <--- 0.1 s ---> [driverID: **2**, pos: 1]

(the divs containing drivers 1&2 information are supposed to change positions in an animated way)

However, since I'm using {1000-pos} as the key attribute, the drivers are no longer considered the unique components. Therefore, React will merely swap the affected components' contents instead of actually rearranging the components. And thus, React-Shuffle won't animate anything because it was not the driver components' order that changed but their contents.

Introducing support for an attribute to order Shuffle's children, or for replacing the proprietary sorting function, could probably solve this issue.

@kenwheeler
Copy link
Contributor

Thanks for the detailed report. I'll look into this and try to get this sorted out for you, no pun intended lol

Harti added a commit to Harti/react-shuffle that referenced this issue Jul 7, 2015
**WARNING!** This is only a draft to enable easier implementation for the project maintainers. Being short on time, I was unable to compile it (I'm pretty new to the ES6 sort of coding), therefore this is **not tested**.

This is only to give you an idea of what I was hinting at in issue FormidableLabs#7, maybe it becomes more clear what I was trying to achieve.
@Harti
Copy link
Author

Harti commented Jul 7, 2015

Please note that I'm still a ES6 noob and don't currently have the setup to easily test if the code referenced in above commit really works.

I just didn't want to throw this issue at you without trying to fix it haha. I believe we will get this sorted though, as long as you don't blindly merge it into the master.

@kenwheeler
Copy link
Contributor

Ok I"ll try it out

@Harti
Copy link
Author

Harti commented Jul 21, 2015

Am I not providing enough information to test it?

This issue is basically a suggestion to enhance the library with one optional, namespaced parameter for specifically declaring the child order. The order is usually taken from the child's key prop, but if your keys are non-sortable (or simply different from the order you want your items to have), an app using react-shuffle cannot behave correctly.

@hayksaakian
Copy link

@Harti that's exactly right. In my case i have a list of video streams, and I want to sort them by the number of viewers.

using the 'key' attribute doesn't work since some of the streams have the same number of viewers, and therefore suffer from key collision if i use the viewer count as the key.

This is what it looks like right now:

sorting incorrectly

@kenwheeler
Copy link
Contributor

So wait, if you use a unique key, such as a UUID, for the purposes of keying, why not sort the children prior to rendering them? The key is only there so React knows which one to move where.

@hayksaakian
Copy link

I just added this to sort the streams but i'm still not seeing it sorted correctly, despite the order looking correct within the JS console

// list is an array of objects that comes from this.props
list.sort(function (a, b){
  if(parseInt(a.rustlers, 10) < parseInt(b.rustlers, 10)){
    return 1;
  }else if(parseInt(a.rustlers, 10) > parseInt(b.rustlers, 10)){
    return -1;
  }
  return 0;
})
console.log('sorted streams', list)
// then I loop over the list and return reactelements 

shuffle sort

@kenwheeler
Copy link
Contributor

And you've confirmed list is sorting properly? You've cast it to an array using React.children or slice?

@hayksaakian
Copy link

@Harti
Copy link
Author

Harti commented Jul 23, 2015

@kenwheeler

So wait, if you use a unique key, such as a UUID, for the purposes of keying, why not sort the children prior to rendering them? The key is only there so React knows which one to move where.

This is exactly what I'm doing as of yet. The latter is not true as (like @hayksaakian pointed out) react-shuffle will only look at the keys, so they're always force sorted by their key. As soon as you wrap the elements in react-shuffle, your rendering order is overruled and you'll never see an actual animation unless you change the key attributes (which may or may not break your application, like I explained in my opening post).

My 'fix' is to introduce an optional (= backwards compatibility) attribute to stop that force sorting.

In case you missed it, I even commited some code. Please do take a look.
Harti@5ce846b

@kenwheeler
Copy link
Contributor

Ok, I forget why that force sorting is taking place. Are you seeing any side effects of removing that sort?

@Harti
Copy link
Author

Harti commented Jul 23, 2015

Removing the sort function altogether might work, but it would break apps that rely on the current sorting (for whatever reason).

I wasn't so sure either, thus I proposed the optional property idea which leaves you with more options. You could leave everything as-is (maintaining old functionality), control the order by explicitly declaring shuffle-order={x} or you could just put shuffle-order="0" to prevent any kind of external reordering.

@kenwheeler
Copy link
Contributor

If removing it works, I'll push that and bump the minor version

@lucasfeliciano
Copy link

👍
I'm very interested in this thread.

I'm having the same issue here.

In my component state I have a ordered list and my key is the component id from our mongodb.
So after I trigger the sort function I change the list in the state, it changes in the "real dom" but the cloned dom remain the same.

I think that should be the best approach to resolve this issue.
Use the state or the array of components to make the sorting, doing this way the developer could control what kind of sort he wants and react-shuffle just apply the changes using its animation

@hayksaakian
Copy link

Thank you for the quick responses @kenwheeler 👍

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

No branches or pull requests

4 participants