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

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

nol13
Copy link

@nol13 nol13 commented Jun 11, 2020

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.

@nol13 nol13 changed the title Fix issues #474 ad #410 - If add new items in correct order Fix issues #474 and #410 - If add new items in correct order Jun 12, 2020
@nol13 nol13 changed the title Fix issues #474 and #410 - If add new items in correct order Fix issues #474 and #410 - add new items in correct order Jun 12, 2020
@taion
Copy link
Member

taion commented Jun 12, 2020

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
Copy link
Author

nol13 commented Jun 12, 2020

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
Copy link
Author

nol13 commented Jun 12, 2020

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
Copy link
Member

taion commented Jun 12, 2020

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
Copy link
Author

nol13 commented Jun 12, 2020

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
Copy link
Member

taion commented Jun 13, 2020

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
Copy link
Author

nol13 commented Jun 14, 2020

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
Copy link
Member

taion commented Jun 14, 2020

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
Copy link
Author

nol13 commented Jun 15, 2020

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
Copy link
Member

taion commented Jun 17, 2020

@jquense @silvenon thoughts on the API here?

@silvenon
Copy link
Collaborator

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

@silvenon
Copy link
Collaborator

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
Copy link
Author

nol13 commented Jun 22, 2020

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
Copy link
Member

taion commented Jun 27, 2020

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
Copy link
Author

nol13 commented Jun 30, 2020

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
Copy link
Member

taion commented Jul 1, 2020

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
Copy link

mogzol commented Jan 21, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants