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!

@robmonie
Copy link

robmonie commented Sep 3, 2024

This change was made quite some time ago but hopefully someone can help with an issue i'm facing which is preventing me from upgrading past 7.6.1.

I've found that since this change was introduced, any code that incorporates useTransition such as modals and popovers fail to render the content of the Modal or popover in tests when using testing library and vitest / jsdom. It may be the same with jest but i'm not sure.

I've been able to isolate it to the moving of setStatus(shouldMount ? "pre-entering" : "pre-exiting"); into requestAnimationFrame and ReactDOM.flushSync. When I revert this, everything works again.

It's worth noting that due to issues with js-dom being flakey with requestAnimationFrame, I needed to stub that as per https://testing-library.com/docs/svelte-testing-library/faq/ so now the code executes but the issue remains.

I'm currently looking into whether this is something I can work around within vitest / react testing library etc but I suspect i'm not the only person to face this and maybe someone else has figured it out?

UPDATE: As usual, asking for help generally helps one solve their own problem :-)

For anyone else coming across this, i've found that combining the requestAnimationFrame stub mentioned above, and writing my tests differently fixes the issue. The request animation frame introduces some asynchronicity into the equation that wasn't being accounted for in the original tests. This is more an issue with the testing tools than mantine.

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
5 participants