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

Batch deletion of removed items #462

Closed
wants to merge 1 commit into from

Conversation

mart-jansink
Copy link

Batch the deletion of removed items after they transitioned out. See #461.

@drcmda
Copy link
Member

drcmda commented Jan 21, 2019

could you checkout the hooks.8.x branch and see if this is still an issue? it's going to get all refactored soon and in the new branch it removes lazily by default already

@mart-jansink
Copy link
Author

I'll give it a try, but I'll need to work out how to use hooks first, so it may take a bit. Or am I missing a way to still use <Transition /> with that branch?

@drcmda
Copy link
Member

drcmda commented Jan 22, 2019

No, for clarity it's been singled out, it will come back when i merge. I can merge this - just give me some time to test it, super busy right now - but i won't forget it.

@grrowl
Copy link

grrowl commented Feb 28, 2019

I'm still seeing this issue on [email protected] using the useTransition hook. Would love to see this merged :)

@drcmda
Copy link
Member

drcmda commented Feb 28, 2019

@grrowl could you make a minimal codesandbox to show the effect?

@grrowl
Copy link

grrowl commented Feb 28, 2019

Sure thing: this demo exhibits the behavior using hooks and the latest react-spring: https://codesandbox.io/s/my869ojmjj

@drcmda
Copy link
Member

drcmda commented Mar 8, 2019

@grrowl could you give me a super quick rundown how you expect it to behave?

@grrowl
Copy link

grrowl commented Mar 10, 2019

Sorry, I didn't explain the problem exhibited by the demo.

When you click "Filter", we re-render with only the item "b". But notice, the item "b" moves into the space taken by "f", even though all the children are keyed and should maintain their position through transitions in and out.

So, what we expect when changing the items array is rendering a list of (using punctuation to illustrate fading:

  • [a] [b] [c] [d] [e] [f] [g] => begin transition out
  • .a. [b] .c. .d. .e. .f. .g. => transition finishes (b maintains position throughout)
  • [b]

The problem is the items are re-ordered when rendered through the "transition" phase as:

  • [a] [b] [c] [d] [e] [f] [g] => begin transition out
  • .a. .d. .e. .f. .g. [b] .c. => transition finishes (b and c move to the end of the transitions array for some reason)
  • [b]

In my example I included the key in item render since, especially when setting unique: true, elements' order should be maintained throughout renders.

edit: my problem is as described in linked pull request #461 — but testing locally (merging mart-jansink:master into master) hasn't seemed fixed the issue. will try to have a stab at this problem or a unit test soon.

@drcmda
Copy link
Member

drcmda commented Mar 10, 2019

yeah, i think batch delete wouldn't have an effect on order, and it actually does batch delete now in some situations. if you want to look at it, that would be amazing as my time's limited right now. it's here somewhere: https://github.com/react-spring/react-spring/blob/master/src/useTransition.js#L250-L259

this is how i debug stuff: https://github.com/react-spring/react-spring-examples/blob/master/README.md

@aleclarson aleclarson closed this Apr 16, 2019
cameron-martin pushed a commit to cameron-martin/react-spring that referenced this pull request May 10, 2022
…rs#462)

Currently, there are some cases that the component doesn't render because it finds that the height is 0. You will see an output like

This can be avoided by providing the desire height for the responsive chart
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants