pmndrs / react-spring

✌️ A spring physics based React animation library
http://www.react-spring.dev/
MIT License
28.21k stars 1.19k forks source link

Child components of Transitions remounts #249

Closed einarlove closed 6 years ago

einarlove commented 6 years ago

I've had an issue where Transition remounts my components constantly even when Transition enter/leave is idle.

I'm tried to boil the problem down to the smallest example even tho it does not quite resemble my issue where I have 20 remounts on one component but non the less it remounts when it shouldn't. Check the console and you'll see that the fourth item unmounts as expected, but gets remounted again.

I tried console.log'ing all over react-spring codebase and saw that Transition was quiet and so was it's rendering, but the Spring within was remounting.

einarlove commented 6 years ago

I updated react, react-dom and react-spring to latest on CodeSandbox and now it works, so I'll come back with how my example break.

einarlove commented 6 years ago

I successfully narrowed it down in a new CodeSandbox. The sinner seems to be animating height: 'auto' while you're within a context shown in the CodeSandbox.

einarlove commented 6 years ago

250 Fixes most of the issues, but I've encountered other bugs related to height: auto not solved.

When doing this:

from={{ opacity: 0, height: 0 }}
enter={{ opacity: 1, height: 'auto' }}
leave={{ opacity: 0 }}

or

from={{ opacity: 0, height: 0 }}
enter={{ opacity: 1, height: 'auto' }}
leave={{ opacity: 0, 'auto' }}

the component never unmounts.

drcmda commented 6 years ago

I noticed the same when trying to fix keyframes, normally springs remember values, but these abstracted primitives seem to have problems dealing with auto as it has to go through multiple async states to properly calculate a value. Surprises me a little since Transition does spread current values before new values to ensure it acts additively: https://github.com/drcmda/react-spring/blob/master/src/Transition.js#L157

einarlove commented 6 years ago

Also had a big problem where my Keyframes within the Transition stopped at random positions everytime and never completed.

einarlove commented 6 years ago

And some problems where height:auto calculated to infinity on leave. The bugs might not be related tho.

drcmda commented 6 years ago

Thanks for the PRs btw, really appreciate it! I'm on it otherwise. Made some progress yesterday and most issues seem to be fixed, including your keyframe [opacity, auto] transition. But i'll have to run some tests first.

drcmda commented 6 years ago

@einarlove Issues should be fixed in 5.9.2, this works now:

from={{ opacity: 0, height: 0 }}
enter={{ opacity: 1, height: 'auto' }}
leave={{ opacity: 0 }}

Of course it'll simply remove the component when it's done since it doesn't shrink that way. Btw, make sead sure you're on React 16.4.x or higher, 16.3.x had a buggy getDerivedStateFromProps implementation (it didn't fire after setState).

This:

from={{ opacity: 0, height: 0 }}
enter={{ opacity: 1, height: 'auto' }}
leave={{ opacity: 0,  'auto' }}

isn't valid javascript, you probably meant something else?

drcmda commented 6 years ago

I've made some steps forward tinkering with array props for transition, this is now wrapping reacts transitiongroup but using react-springs simple API: https://codesandbox.io/s/lr1lrw9mkm

If i can get the same functionality out of this perhaps it would make sense to replace the current implementation. This would also take a major burden off my shoulders not having to manager component states.

I've updated the roadmap, this will definitively be in 6.x https://github.com/drcmda/react-spring/issues/212

einarlove commented 6 years ago

We got our discussion scattered across twitter, pull request and this issue, but I think we should stick with one of them 😅. I suggest here.

@drcmda, I finally got around making a demo explaining the issues I've mentioned before with buggy height:auto calculations and <Keyframes> acting weird when inside the <Transition>.

https://codesandbox.io/s/538pp24yyk

It might seem overcomplicating things, but I tried my best to make the example resemble how it works in my application.

There are 2 problems displayed in the example.

  1. When the gets mounted in the list and gets height animated from 0 → auto, it doubles the height. It only happens to the first element making me believe it's because the parent has no set height/width on first pain(?). Somehow I fixed this in my app by setting explicit width: 500px; and letting children be auto width. It's a hack, not a permanent solution.
  2. The keyframes within the does not animate correctly. It staggers and does not complete by the desired duration spesified. If you move it outside it works. Make it a Spring, it works.
drcmda commented 6 years ago

@einarlove I think i have it down: https://codesandbox.io/embed/kp527wn3r

Your example brought a lot of minor bugs to surface which i haven't seen before, like ordering of deleted items and so on. Also Keyframes had bugs. I've also had to add cancelation to keyframes, which now is possible. All in all, the entire message hub can be driven with a single transition with controlled lifecycles now.