Preserve order in mergeChildMappings

[hellosmithy]

This relates to the issue described here: #6142

I believe this fixes the intended behavior - unless I have misunderstood the old implementations comment and prepending was the desired behavior. It certainly fixes an issue I was having with this when the children are completely replaced with a new array of children.

mergeChildMappings now preserves the order so that new children are appended rather than prepended when all old nodes are removed.

I've also added tests and updated a couple of CSSTransitionGroup tests to reflect this.

[jimfb]

This is a potentially breaking change. But it's not something we could warn for, so I'm not sure there is much we can do about that (assuming we want to take this). I also wonder if the old ordering was intentional - there may be occlusion effects that are desired whereby the new item appears on top of the old.

[hellosmithy]

An alternative might be a prop to set prepend / append if breaking changes are a concern. It seems unusual to prepend only in this situation unless there was a specific reason like you say. There are also no explicit tests to enforce that if it were the case though. Perhaps @spicyj or @petehunt can shed some light on the original intent?

[gaearon]

Merging this as is would most certainly break a lot of styling assumptions for the users of existing TransitionGroup.

To be quite honest I’m not sure it’s even worth introducing as a prop. In animations, relying on DOM order is really flaky. Can you please give more context as to why this is important for your app? Ideally you should be able to specify style={{ zOrder: whatever }} and not worry about the node ordering at all.

[gaearon]

Some info from @petehunt: the original merge algorithm was more “correct” but slower so it was replaced. This could be pluggable if there is enough interest but this would increase the API surface area for something we might not want to commit to long term.

I’m inclined to close this unless you can show a compelling use case for animation that depends on the specific DOM node order that is hard to implement without it.

[hellosmithy]

@gaearon do you mean flex's order property? I've not come across zOrder before.
The use case is an array of logs transitioning in from below. Sometimes the array is completely replaced by a new set of logs, other times logs are appended to the existing array. In all cases the incoming items should transition in from below. When the array is added to React correctly preserves order according to the keys. But currently in the case where the entire array is replaced the incoming items appear above the outgoing items rather than below.

I guess I could switch to flexbox and add the orders inline. Like you say it might introduce a breaking change for some people. But it seemed like odd and unexpected behaviour to me to prepend rather than append by default.

[gaearon]

I meant CSS z-order. I thought you had problems with stacking items and that the new ones appeared below on the Z axis the old ones visually.

I think I understand what you mean better now. You are not animating the position, just some appearance properties (e.g. opacity?). I wonder if you considered positioning the items absolutely based on their index?

I don’t think there is any “correct” position for the new items. In case of logs it makes sense that new items appear on the bottom, but if your app used a reverse timeline (e.g. Twitter), it would make just as much sense for them to appear at the top.

I agree it would be best to configure it. Can you show which properties you animate so I could better understand your use case? Again, I thought you were positioning items absolutely and animating their position so they enter and exit smoothly but this doesn’t appear to be the case.

[hellosmithy]

@gaearon I think changing the source order in CSS could work. I'm not familiar with z-order but I think it's doable like you say.

A very crude example of what I'm trying to do is here which illustrates the order behaving strangely at the end: https://jsfiddle.net/hellosmithy/92he8jge/2/

In my actual use case I have variable height logs so there's a bit more going on. If I position them absolutely then I'll need to do some DOM measuring as there is content beneath. I'm not averse to this although I was trying to avoid it until absolutely necessary. I guess my point is, yes it can be worked around but I made the PR because I was surprised by the default behaviour and even looking at the source code it looked to me like it perhaps wasn't intended behaviour.

I agree there are cases when a reverse order might make more sense e.g. twitter. In those instances wouldn't it make more sense to preserve an append order and then do something like column-reverse rather than manually managing order with inline styles (although that is an option if needed)? The current implementation means that if you have incrementing keys as I do in my case you can't rely on the keys being in any order at all.

[mutley]

@gaearon Are you talking about z-index? From what I can find there is a z-index property and an order property.

[mutley]

I really think allowing this to be configurable through props is the way to go. I also think that having a default prop set so that by default the behavior doesn't change would also be a good thing. Fixing this with CSS is possible by doing something like this http://codepen.io/aardrian/pen/MavVeb but I can see that getting messy quickly.

[gaearon]

Yeah, I meant z-index 😄

Your suggestions make sense but I’m not sure we want to keep supporting TransitionGroup in this repo because we don’t actively use it in production and it’s hard for us to make API decisions without our products actually relying on it.

Let’s keep this open for now. We’ll figure our plan for TransitionGroup and other addons after 15 release and look again at this PR. Thank you for contributing!

[atran]

This is important for SVGs, where stacking order cannot be manipulated via z-order.

[gaearon]

We don’t have an owner for ReactTransitionGroup internally because we don’t use it, and it seems like we don’t have the capacity to review PRs for it at the moment. Your best strategy might be to fork its code (or find an existing fork) and propose these changes there. If any of the forks turns out to be successful we might deprecate our own version and start pointing to the fork in the docs. Sorry it turned out to be this way, but it’s best to do something than to leave this hang forever so I’m closing.