[Drawer] Open/close animation easing and timing (#6059) #6066

ArcanisCz · 2017-02-05T23:52:28Z

Changed easing and duration according to spec, and solved premature modal dialog closing during animation duration.
Fixes #6059

ArcanisCz · 2017-02-05T23:53:37Z

src/Drawer/Drawer.js

@@ -93,6 +98,8 @@ export default class Drawer extends Component {
      children,
      className,
      docked,
+      enterTransitionDuration,


Didnt want to duplicate theese durations here (from Slider), but it turned out, Modal needs this durations to properly show/hide himself

Changed easing and duration according to spec, and solved premature modal dialog closing during animation duration

ArcanisCz · 2017-02-06T00:43:01Z

src/styles/transitions.js

@@ -15,6 +15,19 @@ export const easing = {
  sharp: 'cubic-bezier(0.4, 0.0, 0.6, 1)',
 };

+// Follow https://material.io/guidelines/motion/duration-easing.html#duration-easing-common-durations


Created some constants for transition durations, but on second thought, i think theese should be subject of customization/theme somehow - even documentation gives different durations for different devices (pc, tablet, mobile, watches)

oliviertassinari

Cool PR! Thanks for working on that.
I think that we miss some test. But otherwise, it's almost good to be merged 👍 .

oliviertassinari · 2017-02-06T20:42:18Z

src/Drawer/Drawer.js

+      <Modal
+        {...containerProps}
+        show={open}
+        backdropTransitionDuration={open ? enterTransitionDuration : leaveTransitionDuration}


I would reverse the order to allow high customization power.

+ backdropTransitionDuration={open ? enterTransitionDuration : leaveTransitionDuration} + {...containerProps} + show={open}

oliviertassinari · 2017-02-06T20:43:42Z

src/transitions/Slide.js

@@ -82,14 +85,17 @@ export default class Slide extends Component {
  handleEntering = (element) => {
    const { transitions } = this.context.theme;
    element.style.transition = transitions.create('transform',
-      `${this.props.transitionDuration}ms`);
+      `${this.props.enterTransitionDuration}ms`, undefined, easing.easeOut);


is undefined for 0ms?
I'm wondering if we should change the API. That makes me think of https://twitter.com/brian_d_vaughn/status/828507525469253632.

So, i am thinking of something this:

//all defaults are as now, all, 0ms, ... transitions.create({ properties: 'transform', // String/Array duration: transitions.durations.enteringScreen, // Number - nobody will use anything else than ms anyway delay: ... //as above easing: transitions.easing.easeOut, // String, probably some check if it is from standard functions? });

Where that easing/duration constants would be best to obtain from context theme rather that from static import, since i suspect it would be appealing to change timings according to user agent (mobile/pc, ...)

So, i am thinking of something this:

That's what I have in mind too 👍 . But that's out of the scope of this PR.

oliviertassinari · 2017-02-06T20:45:20Z

src/transitions/Slide.js

    element.style.transform = 'translate3d(0, 0, 0)';
    if (this.props.onEntering) {
      this.props.onEntering(element);
    }
  };

  handleExiting = (element) => {
+    const { transitions } = this.context.theme;
+    element.style.transition = transitions.create('transform',
+      `${this.props.leaveTransitionDuration}ms`, undefined, easing.sharp);


Same, 0ms would probably be more explicit.

changes after CR

added some tests

ArcanisCz · 2017-02-06T22:15:35Z

added tests.

Will try to devise some API for that animation later, for this PR i left it as '0ms' since there are other usages to refactor. Will check that new API with you before refactor for sure 👍

oliviertassinari · 2017-02-06T23:03:08Z

src/Drawer/Drawer.spec.js

+    });
+
+    it('should be passed to to Modal\'s backdropTransitionDuration when open=true', () => {
+      const wrapper = shallow(<Drawer


We rather use that line break pattern.

const wrapper = shallow( <Drawer open /> );

Sorry, was confused by eslint comma-dangle rule there, seemed to be a bit weird.

oliviertassinari · 2017-02-06T23:03:38Z

src/Drawer/Drawer.spec.js

+        enterTransitionDuration={enterDuration}
+        leaveTransitionDuration={leaveDuration}
+      />);
+      assert.strictEqual(wrapper.find(Modal).prop('backdropTransitionDuration'), enterDuration);


Any though on .prop('backdropTransitionDuration')vs.props().backdropTransitionDuration?

no preference, maybe later one is a bit more friendly towards editor's intellisense? Anyway, changed as requested

CR

muibot added the PR: Needs Review label Feb 5, 2017

ArcanisCz commented Feb 5, 2017

View reviewed changes

ArcanisCz force-pushed the nextFeature/Drawer branch from 7a58cfe to 4b747a7 Compare February 6, 2017 00:31

[Drawer] Open/close animation easing and timing (mui#6059)

1ffac61

Changed easing and duration according to spec, and solved premature modal dialog closing during animation duration

ArcanisCz force-pushed the nextFeature/Drawer branch from 4b747a7 to 1ffac61 Compare February 6, 2017 00:31

ArcanisCz commented Feb 6, 2017

View reviewed changes

oliviertassinari reviewed Feb 6, 2017

View reviewed changes

oliviertassinari added next PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Feb 6, 2017

ArcanisCz added 2 commits February 6, 2017 22:26

[Drawer] Open/close animation easing and timing (mui#6059)

f9f8b47

changes after CR

[Drawer] Open/close animation easing and timing (mui#6059)

a36d020

added some tests

muibot added the PR: Needs Review label Feb 6, 2017

oliviertassinari reviewed Feb 6, 2017

View reviewed changes

[Drawer] Open/close animation easing and timing (mui#6059)

d894848

CR

oliviertassinari approved these changes Feb 7, 2017

View reviewed changes

oliviertassinari added PR: Review Accepted and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Feb 7, 2017

oliviertassinari merged commit 7e547a8 into mui:next Feb 7, 2017

ArcanisCz deleted the nextFeature/Drawer branch February 10, 2017 22:21

zannager added the component: drawer This is the name of the generic UI component, not the React module! label Mar 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Drawer] Open/close animation easing and timing (#6059) #6066

[Drawer] Open/close animation easing and timing (#6059) #6066

ArcanisCz commented Feb 5, 2017

ArcanisCz Feb 5, 2017

ArcanisCz Feb 6, 2017

oliviertassinari left a comment

oliviertassinari Feb 6, 2017

oliviertassinari Feb 6, 2017

ArcanisCz Feb 6, 2017 •

edited

oliviertassinari Feb 6, 2017

oliviertassinari Feb 6, 2017

ArcanisCz commented Feb 6, 2017 •

edited

oliviertassinari Feb 6, 2017

ArcanisCz Feb 7, 2017

oliviertassinari Feb 6, 2017

ArcanisCz Feb 7, 2017

[Drawer] Open/close animation easing and timing (#6059) #6066

[Drawer] Open/close animation easing and timing (#6059) #6066

Conversation

ArcanisCz commented Feb 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArcanisCz Feb 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArcanisCz commented Feb 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArcanisCz Feb 6, 2017 •

edited

ArcanisCz commented Feb 6, 2017 •

edited