Fix issues #474 and #410 - add new items in correct order

[nol13]

Fixes #474 and fixes #410

If there are any pending keys left that we don't know where to put, check if there are any new keys that they should be ahead of. Order the pending keys before the first new key that doesn't have a prevKey after it.

[taion]

i'm just catching up on this issue now – this changes the existing behavior for a replace transition in cases where order matters, doesn't it? e.g. in the "single item" case, there would now be no way to get the newly-inserted item to come "before" the item that's being replaced?

i wonder if the right thing here is to use the value of the key itself as a fallback ordering mechanism, or something to that effect. otherwise we're opting all users into one behavior or the other, when in practice there's no way to know which is unambiguously correct

[nol13]

It would change the existing behavior in the case where all of the existing items are removed. If you think of the single item scenario as just a special case of the last item being removed issue, then it would seem to make sense like this. I know in my use case where we have a list of a bunch of items but are only showing say 25 at a time, when the old items leave you'd want the new ones that enter to fill the missing places to come in from the bottom rather than the top, as the old ones animate away.

So if I exit all 25 current items, the new ones that come after it in the list enter from the bottom. This is what it does currently when new items enter, except that if the last existing item leaves, it never comes across any more existing items to place them before. So if a middle item exits, the new one goes on bottom, but if the last item exits, the new item gets added and then the previously last pending item is added behind it.

But ya there could potentially be some code out there that depends on the old behavior. Should #410 be considered a bug? Like would there be some prop that says, "if all items are replaced, place the new items before the old ones, but if only some of them are replaced, they go in list order"

Could use key as fallback, but has there been any assumption that the keys were ordered until now? Seems like that might break existing code as well. Maybe could be handled at the level of each individual transition?

[nol13]

Currently definition of this function from the comment says "This function takes a previous set of keys and a new set of keys and merges them with its best guess of the correct ordering," and this seems like a reasonable guess, but ya I could see where it might be useful to have more control over it.

[taion]

right; the problem is that if we go from [a] to [b], we don't know whether the animation state should be [a, b] or [b, a].

i assume you're actually hitting this problem. what sort of API would you find useful for controlling this?

i wonder if we want something like:

<TransitionGroup.Child key="b" placementHint="prepend">
  <MyComponent />
</TransitionGroup.Child>

[nol13]

Ya I think that would work splendidly.

Just trying to think, if, for some reason, you replaced an item in the list with 2 new items, the first one append and then 2nd prepend, how to handle it. Go by the first one in the group?

Or since you might want to put something before and after, prepends before until the first append, and everything after the append after. Or put them in the placement as specified, and if they end up in a different order than originally given in, then they shouldn't have done that.

[taion]

Like this, I think:

<TransitionGroup ...>
  <TransitionGroup.Child key="a" rangeBehavior="prepend">
    <Foo />
  </TransitionGroup.Child>
  <TransitionGroup.Child key="b" rangeBehavior="append">
    <Foo />
  </TransitionGroup.Child>
</TransitionGroup>
```js

Note that this would only really matter in the case where we remove all keys. Everything with `prepend` goes before the removed keys, while everything with `append` goes after.

[nol13]

Should work, I think could probably implement it.

The "only matters when you remove all keys" part might not apply anymore though, since it could also be used when replacing items in the middle of a list.

[taion]

Yeah, that could work too. I suspect in practice you might still need to do some key merging, though... I'd do whatever ends up easier to implement.

[nol13]

Yup, if you replaced consecutive keys and wanted to interleave the new ones it still wouldn't be able to. Was thinking there could be a 'replacesKey' prop, or go back to the fallback ordering you mentioned or some other way to directly specify. I personally have no need for it at the moment at least.

So this is updated to add a new boolean prop appendOnReplace to the transitions. If it's not set then it will default to prepend and the behavior should be the same as before without any breaking changes. If there are no replacement items then it will get ignored as well, and only apply when determining whether pending items should go before or after new items.

Could change back to string but the bool seemed to work with only the 2 possible values.

[taion]

@jquense @silvenon thoughts on the API here?

[silvenon]

I'll check this PR out on Saturday, thanks! 🙂 📅

[silvenon]

What a tricky issue! At first I thought that we could add an allChildKeys prop to TransitionGroup so that we could reach more expected conclusions when mapping children, but that way we could solve only the issue where an array is being sliced, not when an entire list is being replaced with a new one and a myriad of other cases you mentioned. My next thought was to provide a some sort of a prop which takes a function with the old list and the new list, but that would just boil down to TransitionGroup's internals.

This issue seems a little beyond me at the moment, but I'll let you know if anything comes to mind.

[nol13]

If it's important to have the full control, was thinking there could be a replacesKey prop that would work as an additional hint to appendOnReplace, where if it's replacing a section of existing keys, it will look if the value for its 'replacesKey' exists in the keys it will replace, and if so use that to determine the exact key that it should go before or after.

Or, simplify it and don't worry about the case where you have some elements that would go before and some after, and make it a top level prop on TransitionGroup.

[taion]

Okay, what if instead of trying to be clever here, we just let the user pass in a custom callback that will replace our mergeChildMappings is specified?

It's not the best API ever, but it should solve any potential customization use case.

[nol13]

Would work. Feel like a top level prop for default direction would solve most people issues without having to re-implement internals themselves, but ya not as flexible. Would be happy to try to implement it either way.

Fwiw for what I was doing it was working great with 25 and 50 items, but of course we wanted to start animating ~100 elements in and out at a time and hit some performance issues in Firefox, so had to switch to a 2 step out then in approach. Did try the fast-dom fork too, but doing a bunch of crazy stuff callbacks and such probably wasn't helping. ;)

[taion]

I was thinking that we'd export the "prepend instead of append" variant to satisfy the basic use case, but still expose the lower-level API just in case.

I dunno, just a thought – @silvenon @jquense?

[mogzol]

Are there any updates for this? I'm running into animation issues in my app that are caused by this, and currently don't really have any way to fix them. The current PR would work great, but any of the other proposed solutions would be fine too, I just need some way to fix it.