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] Make paper ref accessible #35082

Merged
merged 8 commits into from Nov 24, 2022

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Nov 10, 2022

Fixes: #35069

@mui-bot
Copy link

mui-bot commented Nov 10, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-35082--material-ui.netlify.app/

Details of bundle changes

Generated by 🚫 dangerJS against 7c6bb24

@zannager zannager added component: drawer This is the name of the generic UI component, not the React module! component: SwipeableDrawer The React component. labels Nov 10, 2022
@sai6855 sai6855 changed the title [SwipeableDrawer] made paper ref accesiable [SwipeableDrawer] made paper ref accessible Nov 10, 2022
@sai6855 sai6855 marked this pull request as draft November 14, 2022 12:40
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

The fix should be as simple as doing:

diff --git a/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js b/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js
index 89a02141c8..29eed9dd4c 100644
--- a/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js
+++ b/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js
@@ -574,7 +574,7 @@ const SwipeableDrawer = React.forwardRef(function SwipeableDrawer(inProps, ref)
             pointerEvents: variant === 'temporary' && !open ? 'none' : '',
             ...PaperProps.style,
           },
-          ref: paperRef,
+          ref: PaperProps.ref || paperRef,
         }}
         anchor={anchor}
         transitionDuration={calculatedDurationRef.current || transitionDuration}

@ZeeshanTamboli ZeeshanTamboli changed the title [SwipeableDrawer] made paper ref accessible [SwipeableDrawer] Make paper ref accessible Nov 19, 2022
@ZeeshanTamboli ZeeshanTamboli added the bug 🐛 Something doesn't work label Nov 19, 2022
@sai6855 sai6855 marked this pull request as ready for review November 19, 2022 11:00
@sai6855
Copy link
Contributor Author

sai6855 commented Nov 19, 2022

Thanks for reviewing @ZeeshanTamboli 🙏, I've updated code as described.

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Nov 19, 2022

@sai6855 Actually I think your previous logic was correct:

diff --git a/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js b/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js
index 89a02141c8..c786d47a40 100644
--- a/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js
+++ b/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js
@@ -166,7 +166,9 @@ const SwipeableDrawer = React.forwardRef(function SwipeableDrawer(inProps, ref)

   const swipeAreaRef = React.useRef();
   const backdropRef = React.useRef();
-  const paperRef = React.useRef();
+  const localPaperRef = React.useRef();
+
+  const paperRef = PaperProps.ref || localPaperRef;

   const touchDetected = React.useRef(false);

Because paperRef.current is used in the code below in multiple places and if custom ref is provided via PaperProps.ref it should consider that which does not happen with this change.

I apologize for the mistake.

Test looks good.

@ZeeshanTamboli ZeeshanTamboli dismissed their stale review November 19, 2022 13:37

Suggested test now applied.

@ZeeshanTamboli
Copy link
Member

The fix should be as simple as doing:

diff --git a/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js b/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js
index 89a02141c8..29eed9dd4c 100644
--- a/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js
+++ b/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js
@@ -574,7 +574,7 @@ const SwipeableDrawer = React.forwardRef(function SwipeableDrawer(inProps, ref)
             pointerEvents: variant === 'temporary' && !open ? 'none' : '',
             ...PaperProps.style,
           },
-          ref: paperRef,
+          ref: PaperProps.ref || paperRef,
         }}
         anchor={anchor}
         transitionDuration={calculatedDurationRef.current || transitionDuration}

This is incorrect

@sai6855
Copy link
Contributor Author

sai6855 commented Nov 19, 2022

@sai6855 Actually I think your previous logic was correct:

diff --git a/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js b/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js
index 89a02141c8..c786d47a40 100644
--- a/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js
+++ b/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js
@@ -166,7 +166,9 @@ const SwipeableDrawer = React.forwardRef(function SwipeableDrawer(inProps, ref)

   const swipeAreaRef = React.useRef();
   const backdropRef = React.useRef();
-  const paperRef = React.useRef();
+  const localPaperRef = React.useRef();
+
+  const paperRef = PaperProps.ref || localPaperRef;

   const touchDetected = React.useRef(false);

Because paperRef.current is used in the code below in multiple places and if custom ref is provided via PaperProps.ref it should consider that which does not happen with this change.

I apologize for the mistake.

Test looks good.

okay @ZeeshanTamboli no issues, so according to my previous logic as typeof paperRef would be "any", because of this type "any" eslint is suggesting to include paperRef in dependency array. So is it fine if i include paperRef in useEffect in dependency array or do you have any other solution in mind?

  const localPaperef = React.useRef();
  const paperRef = PaperProps.ref || localPaperef;

Screenshot 2022-11-19 at 7 52 18 PM

@ZeeshanTamboli
Copy link
Member

because of this type "any" eslint is suggesting to include paperRef in dependency array. So is it fine if i include paperRef in useEffect in dependency array or do you have any other solution in mind?

It's fine to include it in the dependency array if eslint complains about it. But I think actually the paperRef value never changes.

Signed-off-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>
Signed-off-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

LGTM!

@ZeeshanTamboli ZeeshanTamboli merged commit 1b86330 into mui:master Nov 24, 2022
daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
Signed-off-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>
Co-authored-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>
feliperli pushed a commit to jesrodri/material-ui that referenced this pull request Dec 6, 2022
Signed-off-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>
Co-authored-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: drawer This is the name of the generic UI component, not the React module! component: SwipeableDrawer The React component.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SwipeableDrawer] Passing ref through PaperProps does not set reference
5 participants