[SwipeableDrawer] Only trigger a swipe when appropriate #17993
Conversation
|
@material-ui/core: parsed: +0.29% , gzip: +0.38% Details of bundle changes.Comparing: 36b8ac9...34c3c50
|
| @@ -201,6 +278,19 @@ const SwipeableDrawer = React.forwardRef(function SwipeableDrawer(props, ref) { | |||
| const currentX = calculateCurrentX(anchorRtl, event.touches); | |||
| const currentY = calculateCurrentY(anchorRtl, event.touches); | |||
|
|
|||
| if (open && paperRef.current.contains(event.target)) { | |||
There was a problem hiding this comment.
What do you think of introducing a nodeWhoClaimedTheScroll like variable back? I think that we can improve the UX by disabling the swipe for all the touch session if we have found one native scroll handler. Does it make sense, I can provide more detail if needed.
There was a problem hiding this comment.
By not doing the touch start, everything works just fine. I don't see why introducing a global lock would help.
There was a problem hiding this comment.
I have observed two issues using this logic:
- Once you reach the end of the native scroll, the drawer starts to drag. If you look at the behavior of nested scroll containers, you would need to reach the end of the scrollbar with the nested container and start a new interaction to scroll the parent. In this demo, I don't think that the drawer should start to drag. It's the behavior implemented react-swipeable-views, I think that it's more intuitive/less disturbing.
- Because of 1., something else happens:
There was a problem hiding this comment.
In this demo, I don't think that the drawer should start to drag.
I liked that behavior, but I see why it may not be desired. Regarding point 2: oh... Allright, 1. should fixed.
There was a problem hiding this comment.
Regarding 1. I have a strong conviction that we should allow a single scroll container, per touch session (start, move, end).
I'm no longer sure that 2. is strictly related to 1. I think that it requires further investigation.
There was a problem hiding this comment.
I fixed both issues by re-introducing nodeThatClaimedTheSwipe. It isn't used for inter-drawer locking anymore, but for drawer vs. scrollable container inside that drawer locking, that's why it's updating while moving (to fix the behavior in your gif).
|
@oliviertassinari I can't get the tests to work. 😭 The touchstart events need to be raised on the drawer's paper (or the backdrop; do fulfill the new target check) as target if the drawer is open (that's simple to do), but I can't get a reference to that paper/backdrop somehow. |
|
@leMaik Let met know if you need help with this effort. I should be able to find the time next week. |
…ntainers in drawers taking over the swipe
Thank you! 👍 I looked into the tests again and just fixed them by adding an |
|
Awesome, I will have a look at the changes later today. I had a quick look, I might have spotted some possible edge cases, but nothing confirmed yet. |
oliviertassinari
changed the title
|
@leMaik Well done 👏 |
oliviertassinari
added
the
component: drawer
Fixes #17408, using @oliviertassinari's proposed solution.
Also, this fixes #16942, using the approach suggested here, taken from react-swipeable-views and simplified/adapted for the swipeable drawer.
I changed the temporary swipeable drawer demo to show the reproduction content on all sides, here's a video (Github doesn't let me upload webm files and the gif was way too big): demo.webm.zip