[SwipeableDrawer] Fix React 18 issues #34505
Conversation
| @@ -234,7 +235,9 @@ const SwipeableDrawer = React.forwardRef(function SwipeableDrawer(inProps, ref) | |||
| } | |||
| claimedSwipeInstance = null; | |||
| touchDetected.current = false; | |||
| setMaybeSwiping(false); | |||
| flushSync(() => { | |||
There was a problem hiding this comment.
Fixes the part of the issue where the partially opened drawer (> hysteresis) is not opening the drawer completely. Could probably go as a standalone fix.
| flushSync(() => { | ||
| setMaybeSwiping(true); | ||
| }); | ||
|
|
||
| if (!open && paperRef.current) { |
There was a problem hiding this comment.
I assumed that here paperRef.current will be defined for sure because of the flushSync before, which would change the setMaybeSwiping state, which is used for setting the open prop on the Drawer, and hence the modal. However, this is not the case, so I needed to add the keepMounted prop on the ModalProps, to be sure that the ref would be assigned to a node.
There was a problem hiding this comment.
We use ModalProps={{ keepMounted: true }} anyways at the moment
|
@eps1lon I requested review from you in case you have time to look in the changes in the PR, no hard feelings if you don't have time :) Maybe you will have a better understanding on why |
|
All I remember is that the implementation of the drawer and swipeable-views was always a huge headache. Even before React 18. So this would require an extensive deep dive to understand why Probably best to rewrite this component from scratch (ignoring unit tests and focusing on e2e behavior first) to ensure its compatibility with React 18. |
Alright, thanks for giving the insights. We will keep this in mind when working on the headless hook 👍 |
|
cc @michaldudak |
packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.test.js
Outdated
Show resolved
Hide resolved
packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.test.js
Outdated
Show resolved
Hide resolved
Signed-off-by: Marija Najdova <mnajdova@gmail.com>
|
Thank you @mnajdova for the fix. We are experiencing the issue in our app, where we use |
@lukaselmer it will be available with the release next week. |
|
Great, thank you @mnajdova @michaldudak! |
|
Hi, this is changing default behaviour of SwipeableDrawer component that by default until now (5.10.11) it had ModalProps={{ keepMounted: false }} and now (5.10.12) this is true. |
|
@JoseBuendiaDigio thanks for pointing this out. We missed the docs. I've just created a PR to address this: #35076 |
Fixes #30414
Steps to reproduce:
❌ Current master Sandbox
✅ This PR Sandbox
Remarks
Not happy about the
keepMounted, but if that is not set, thepaperRef.currentisundefinedwhen the touch start event on the body is invoked. At first I thought usingflushAsyncon the set state before the check would be sufficient, as the DOM should be updated after the callback is being invoked, at least that is how I understand based on the description https://reactjs.org/docs/react-dom.html#flushsync, but that's not the case. Interestingly, using onlykeepMountedwithout theflushSyncdoesn't work too.