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

Child components of Transitions remounts #249

Open
einarlove opened this Issue Oct 3, 2018 · 11 comments

Comments

Projects
None yet
2 participants
@einarlove
Contributor

einarlove commented Oct 3, 2018

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

This comment has been minimized.

Show comment
Hide comment
@einarlove

einarlove Oct 3, 2018

Contributor

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.

Contributor

einarlove commented Oct 3, 2018

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 einarlove closed this Oct 3, 2018

@einarlove

This comment has been minimized.

Show comment
Hide comment
@einarlove

einarlove Oct 3, 2018

Contributor

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.

Contributor

einarlove commented Oct 3, 2018

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 einarlove reopened this Oct 3, 2018

einarlove added a commit to einarlove/react-spring that referenced this issue Oct 3, 2018

@einarlove

This comment has been minimized.

Show comment
Hide comment
@einarlove

einarlove Oct 3, 2018

Contributor

#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.

Contributor

einarlove commented Oct 3, 2018

#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

This comment has been minimized.

Show comment
Hide comment
@drcmda

drcmda Oct 3, 2018

Owner

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

Owner

drcmda commented Oct 3, 2018

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

@drcmda drcmda added the bug label Oct 3, 2018

@einarlove

This comment has been minimized.

Show comment
Hide comment
@einarlove

einarlove Oct 4, 2018

Contributor

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

Contributor

einarlove commented Oct 4, 2018

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

@einarlove

This comment has been minimized.

Show comment
Hide comment
@einarlove

einarlove Oct 4, 2018

Contributor

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

Contributor

einarlove commented Oct 4, 2018

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

@drcmda

This comment has been minimized.

Show comment
Hide comment
@drcmda

drcmda Oct 4, 2018

Owner

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.

Owner

drcmda commented Oct 4, 2018

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

This comment has been minimized.

Show comment
Hide comment
@drcmda

drcmda Oct 4, 2018

Owner

@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?

Owner

drcmda commented Oct 4, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@drcmda

drcmda Oct 4, 2018

Owner

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 #212

Owner

drcmda commented Oct 4, 2018

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 #212

@einarlove

This comment has been minimized.

Show comment
Hide comment
@einarlove

einarlove Oct 8, 2018

Contributor

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.
Contributor

einarlove commented Oct 8, 2018

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

This comment has been minimized.

Show comment
Hide comment
@drcmda

drcmda Oct 9, 2018

Owner

@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.

Owner

drcmda commented Oct 9, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment