-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: add exitBeforeEnter prop to useTransition #1773
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
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pmndrs/react-spring-io/HqAnbst9t1aJyY9N3Q8Ya4RfeLCd |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit cde9a28:
|
I'll write up the documentation when the PR is approved, just incase something needs editing for whatever reason. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! New demo looks great too.
I only had the one question in my comment and also there are a few commented out console.log
s that I don't know if you want to keep in there.
/** | ||
* Remove the exited transition from the list | ||
*/ | ||
exitingTransitions.current.delete(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be in the if (exitBeforeEnter)
check below? It doesn't seem to be affecting the functionality in the demo or anything, but as far as I can tell, exitingTransitions
doesn't get populated unless exitBeforeEnter === true
(I also may be missing something though 😅 ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. But if someone changes the prop we might still have it I think... so maybe we should be checking if t exists. Although according to MDN it just returns false
if it didn't exist....
So it won't break, but maybe it's a more explicit thing to do?
ref: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/delete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats a good point about changing the prop -- since later in the hook exitingTransition.current.size
is used to check in the useLayoutEffect
. I assume we'd want it to be empty no matter what then? And if it just returns false
it won't affect anything else.
I think with that in mind it is kind of explicit -- "We want this removed when done, no matter what."
Thanks for catching those rogue logs! I've removed them so going to move forward with this PR. |
Why
This PR adds the
exitBeforeEnter
prop to theuseTransition
hook as requested here #1064. It also adds a new demo showcasing the new prop used with aSpringRef
.What
Adds the prop and splits the changes into exiting and not is
exitBeforeEnter = true
. This works very well when noSpringRef
is passed to theuseTransition
hook.The problem when using a ref is the ref parent component can't be flagged for re-render so the users API cannot call start to bring in the entering animation after the exiting has left. To solve this issue, we flag a
forceChange
whenforceUpdate
is called so instead of updating the queue of the passed ref, we instead run the animation straight away.This is inline with how calling
start
works with the prop turned off, the animations overlap – leave. & enter happen at the same time. So I think this is an acceptable solution.Also added a test for this feature.
Checklist