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

[@mantine/core] next try fixing transition issues #5873

Merged

Conversation

cyantree
Copy link
Contributor

@cyantree cyantree commented Mar 5, 2024

(Hopefully) fixes #3126 #5193 #5849.

As I found out earlier it's probably related to the automatic batching in React. Therefore this fix uses flushSync() to enforce the state update at the right moment.

I hope that this will fix the issue.

@cyantree cyantree force-pushed the issue-3126_next-try-fixing-transition branch from 71ab7e9 to 40db9b6 Compare March 5, 2024 22:23
Copy link
Contributor

@rilrom rilrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @cyantree! I can no longer reproduce the issue.

I'm curious why the changes to modal tests were necessary?

@cyantree
Copy link
Contributor Author

cyantree commented Mar 6, 2024

I think that's because at first the component is unmounted and therefore the state is exited. This leads to the children not being rendered at all. This in effects leads to the test failing.
https://github.com/mantinedev/mantine/blob/master/packages/%40mantine/core/src/components/Transition/Transition.tsx#L71

This is because with my changes the state transition from exited to pre-entering doesn't happen in the same frame anymore (this is done because ReactDOM.flushSync() can't be used within a running render pass).

So setting transitionDuration: 0 disables the transition and renders the children directly. IMHO that's an acceptable behaviour.

However I'm happy for any suggestions as this (at least for me) is a finicky bug.

@macjuul
Copy link

macjuul commented Mar 8, 2024

I can also confirm it solves the problem on my end, thanks so much!

@rtivital rtivital merged commit 7a616fc into mantinedev:master Mar 12, 2024
1 check passed
@rtivital
Copy link
Member

Thanks everyone for validation! @cyantree, thanks for the fix!

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.

Transition resolves instantly sometimes
4 participants