mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.96k stars 32.27k forks source link

[SwipeableDrawer] Add an hysteresis property #12696

Closed jniclas closed 6 years ago

jniclas commented 6 years ago

To make the swipable drawer more natural, i suggest a small change in "material-ui/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js"

Expected Behavior

currently the drawer only chances the state when the swipe is min the half of the width of the drawer. It would be more natural if it would depend on the direction and not on the width of the swipe. Simply change the "if" statement on line 222 in "material-ui/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js" from "translateRatio > 0.5" to "_this.startX > current"

Current Behavior

Examples

Context

oliviertassinari commented 6 years ago

@jniclas At first sight, this suggestion goes against how the other implementations are handling the problem. Wouldn't that introduce many false positive (wrong intent of swipe)?

jniclas commented 6 years ago

@oliviertassinari its simply the approach with fewest changes :) It would not make false positives, cause the already implemented swipeAreaWidth still works

oliviertassinari commented 6 years ago

On react-swipeable-views we have a hysteresis property to control the threshold. 50% isn't always the best limit. If I understand your use case correctly, you want to set 1%.

oliviertassinari commented 6 years ago

@jniclas Do you want to work on such hysteresis property?

  /**
   * Configure hysteresis between slides. This value determines how far
   * should user swipe to switch slide.
   */
  hysteresis: PropTypes.number,

https://github.com/oliviertassinari/react-swipeable-views/blob/510f6502da265469b503bfa4bc46beb9f43824ec/packages/react-swipeable-views/src/SwipeableViews.js#L899-L903

jniclas commented 6 years ago

Yes, that was exactly my thought! I would really like to implement this prop on SwipeableDrawer

oliviertassinari commented 6 years ago

@jniclas Feel free to work on it. We will review your work.

leMaik commented 6 years ago

While a hysteresis prop is a great addition, using the velocity instead of the amount the drawer is opened would be even better. ~Unfortunately, that would make histeresis obsolete and thus might result in a breaking change.~

Which way do we want to take? Velocity or position? :thinking:

oliviertassinari commented 6 years ago

Which way do we want to take? Velocity or position? 🤔

Why can't we do both?

leMaik commented 6 years ago

@oliviertassinari I came to the same conclusion, that's why I stroke that incompatibility sentence. :+1:

jniclas commented 6 years ago

I just implemented it with the method of measuring the width of the slide in the pull request #12722, and it feels quite natural! It could be extended with some velocity measurement.

jniclas commented 6 years ago

I just added a velocity threshold prop to pull request #12722. Now it feels very native (I think). Maybe the property name "velocity" is not suitable?! Maybe something with "...threshold"?

leMaik commented 6 years ago

@jniclas Awesome! :heart: I wonder if we should enable velocity by default. @mbrookes once correctly noticed that "currently the drawer is more a draggable drawer than a swipeable drawer"¹ Adding velocity fixes this.

Regarding the value: Android's drawer (at least the one in the support library) seems to use a threshold of 400 dp/sec, which would be 400 px/sec in a browser? At least that's how I understand the code.

The name… swipeVelocityThreshold?

¹ not the exact wording, but I remember something around that :sweat_smile: