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

[SwipeableDrawer] swipe action doesn't work with preact #18977

Closed
2 tasks done
tky5622 opened this issue Dec 25, 2019 · 9 comments · Fixed by #19782
Closed
2 tasks done

[SwipeableDrawer] swipe action doesn't work with preact #18977

tky5622 opened this issue Dec 25, 2019 · 9 comments · Fixed by #19782
Labels
component: drawer This is the name of the generic UI component, not the React module! component: SwipeableDrawer The React component. external dependency Blocked by external dependency, we can’t do anything about it good first issue Great for first contributions. Enable to learn the contribution process. preact

Comments

@tky5622
Copy link

tky5622 commented Dec 25, 2019

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Swiping to open or close drawer don't work with preact.
only works swipe area's touch event.
In case of using react with same code, drawer works properly.

Expected Behavior 🤔

swipeable drawer can detect touch event and handle state.

Steps:

  1. use preact x
  2. set swipeable drawer
  3. reproduce it

Context 🔦

Your Environment 🌎

"@material-ui/core": "^4.8.1",
"next": "^9.1.7-canary.2",
"typescript": "^3.7.3",
"preact": "^10.1.1",
"preact-render-to-string": "^5.1.3",

reproduction

sandbox (swipe is not work in sandbox editor)
https://codesandbox.io/s/exciting-worker-i67ug

[for reproduction] please swipe from right in this page
https://i67ug.sse.codesandbox.io/index

@tky5622 tky5622 changed the title SwipeableDrawer don't work with preact SwipeableDrawer's swipe action doesn't work with preact Dec 25, 2019
@tky5622 tky5622 changed the title SwipeableDrawer's swipe action doesn't work with preact [SwipeableDrawer] swipe action doesn't work with preact Dec 25, 2019
@oliviertassinari oliviertassinari added external dependency Blocked by external dependency, we can’t do anything about it component: SwipeableDrawer The React component. labels Dec 25, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 25, 2019

@tky5622 Could it be a bug in Preact? Unrelated to Material-UI?

@tky5622

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@tky5622
Copy link
Author

tky5622 commented Dec 25, 2019

When I said "with Preact", I meant as unrelated to Material-UI.

I still don't know the exact cause of it.
However, it may be similar to the issue this pull request solved.
#18027

if synthetic event is used in swipe action, the problem is not with preact.

@oliviertassinari oliviertassinari added waiting for 👍 Waiting for upvotes and removed waiting for 👍 Waiting for upvotes labels Jan 11, 2020
@oliviertassinari
Copy link
Member

As far as I could dive into it, the issue comes from preact not firing the ref, in:
https://github.com/mui-org/material-ui/blob/a992b09f1c0c7d4e16ceb8b1982242c5a1dd6d7d/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js#L530

Without the reference to the paper DOM element, nothing works. I don't think that we can do more about it here. I would encourage you to report the issue on Preact side.

@developit
Copy link

developit commented Feb 11, 2020

@oliviertassinari what's the reason for manually recreating object refs using findDOMNode? That seems like a strange hack, and likely the source of preactjs/preact#2256.

https://github.com/mui-org/material-ui/blob/a992b09f1c0c7d4e16ceb8b1982242c5a1dd6d7d/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js#L502-L510

@oliviertassinari
Copy link
Member

@developit I think that the motivation was to support the case when a developer provides a custom Backdrop that doesn't use React.forwardRef ref correctly. We have an open discussion about it on the React repo: facebook/react#15091.

In the case of the Paper, it seems to be legacy, we can drop it :). Nice finding!

@oliviertassinari oliviertassinari added the ready to take Help wanted. Guidance available. There is a high chance the change will be accepted label Feb 11, 2020
@oliviertassinari
Copy link
Member

@tky5622 Do you want to solve this problem? I think that we could move forward with this simple fix :)

diff --git a/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js b/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js
index ddd7944600..0d59b86719 100644
--- a/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js
+++ b/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js
@@ -504,11 +504,6 @@ const SwipeableDrawer = React.forwardRef(function SwipeableDrawer(props, ref) {
     backdropRef.current = ReactDOM.findDOMNode(instance);
   }, []);

-  const handlePaperRef = React.useCallback(instance => {
-    // #StrictMode ready
-    paperRef.current = ReactDOM.findDOMNode(instance);
-  }, []);
-
   return (
     <React.Fragment>
       <Drawer
@@ -527,7 +522,7 @@ const SwipeableDrawer = React.forwardRef(function SwipeableDrawer(props, ref) {
             pointerEvents: variant === 'temporary' && !open ? 'none' : '',
             ...PaperProps.style,
           },
-          ref: handlePaperRef,
+          ref: paperRef,
         }}
         anchor={anchor}
         transitionDuration={calculatedDurationRef.current || transitionDuration}

@oliviertassinari oliviertassinari added good first issue Great for first contributions. Enable to learn the contribution process. and removed ready to take Help wanted. Guidance available. There is a high chance the change will be accepted labels Feb 11, 2020
@TommyJackson85
Copy link
Contributor

TommyJackson85 commented Feb 19, 2020

I will take give this a go as I am working on this feature lately and nobody has done anything on this yet. Check the PR I created:

@oliviertassinari oliviertassinari added the component: drawer This is the name of the generic UI component, not the React module! label Nov 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: drawer This is the name of the generic UI component, not the React module! component: SwipeableDrawer The React component. external dependency Blocked by external dependency, we can’t do anything about it good first issue Great for first contributions. Enable to learn the contribution process. preact
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants