feat(ripple): Implement subset of improved interaction response guidelines #419

traviskaufman · 2017-03-21T17:03:46Z

Background

When the ripple was originally designed from a motion perspective,
bounded and unbounded activation/deactivation used two different
animation styles, distinct from one another.

The UX updates to the ripple remove this distinction of bounded vs.
unbounded solely with regard to how the ripple animates in and out.
There is still a distinction between bounded vs. unbounded, but we no
longer have to account for this when animating the ripple. Thus, there
are no API changes; only UX changes.

Changes

Implement UX changes for "tap" + "tap and hold" interactions
Remove all references of "bounded" vs. "unbounded" from
activation/deactivation code

Resolves #190

traviskaufman · 2017-03-21T17:08:43Z

@cristobalchao if you could do a quick pass on the UX here I'd appreciate it!

codecov-io · 2017-03-21T17:13:31Z

Codecov Report

Merging #419 into master will increase coverage by 0.13%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #419      +/-   ##
==========================================
+ Coverage   99.02%   99.16%   +0.13%     
==========================================
  Files          40       40              
  Lines        2058     2036      -22     
  Branches      264      260       -4     
==========================================
- Hits         2038     2019      -19     
+ Misses         20       17       -3

Impacted Files	Coverage Δ
packages/mdc-ripple/constants.js	`100% <ø> (ø)`	⬆️
packages/mdc-ripple/foundation.js	`99.48% <100%> (+1.44%)`	⬆️
packages/mdc-ripple/util.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6dbe84...322f2b6. Read the comment docs.

BBosman · 2017-03-21T18:26:29Z

packages/mdc-ripple/_keyframes.scss

@@ -20,46 +20,33 @@

 @keyframes mdc-ripple-fg-radius-in {
  from {
-    transform: translate(0) scale(1);


Why is this removed? Don't we need a fallback for browsers that don't support var?

@BBosman good question. Typically, yes. However, this keyframe is only attached when mdc-ripple-upgraded is added to the ripple surface. Before we add that class, we check - within JS - to make sure custom properties are supported. Thus, fallbacks here aren't necessary.

...I'm going to add this explanation as a comment 😃

BBosman · 2017-03-21T18:26:55Z

packages/mdc-ripple/_keyframes.scss

@@ -20,46 +20,33 @@

 @keyframes mdc-ripple-fg-radius-in {
  from {
-    transform: translate(0) scale(1);
    transform: translate(var(--mdc-ripple-fg-translate-start, 0)) scale(1);
    animation-timing-function: $mdc-animation-fast-out-slow-in-timing-function;
  }

  to {


BBosman · 2017-03-21T18:27:44Z

packages/mdc-ripple/_mixins.scss

-      mdc-ripple-fg-unbounded-transform-deactivate var(--mdc-ripple-fg-unbounded-transform-duration, 0) $mdc-animation-fast-out-slow-in-timing-function;
+  &.mdc-ripple-upgraded--foreground-deactivation#{$pseudo} {
+    // Retain transform from mdc-ripple-fg-radius-in activation
+    transform: translate(var(--mdc-ripple-fg-translate-end, 0)) scale(var(--mdc-ripple-fg-scale, 1));


yeelan0319 · 2017-03-23T13:47:31Z

packages/mdc-ripple/constants.js

@@ -15,42 +15,33 @@
 */

 export const ROOT = 'mdc-ripple';
-export const UPGRADED = `${ROOT}-upgraded`;


nit: It seems like the exported const ROOT and UPGRADED is not being used anywhere.

traviskaufman · 2017-03-24T19:01:23Z

@yeelan0319 Changes made PTAL!

yeelan0319 · 2017-03-27T16:35:00Z

packages/mdc-ripple/_keyframes.scss

    transform: translate(var(--mdc-ripple-fg-translate-start, 0)) scale(1);
    animation-timing-function: $mdc-animation-fast-out-slow-in-timing-function;
  }

  to {
-    transform: translate(0) scale(0);
-    transform: translate(var(--mdc-ripple-fg-translate-end, 0)) scale(var(--mdc-ripple-fg-scale, 0));
+    transform: translate(var(--mdc-ripple-fg-translate-end, 0)) scale(var(--mdc-ripple-fg-scale, 1));


Is there any reason why the default value is different from --mdc-ripple-fg-scale default value (which is 0 and default to 0 else where in _mixins.scss). Is this a spec change?

Looks like an oversight on my part for the --mdc-ripple-fg-scale default value, which should be set to 1. Will change.

yeelan0319 · 2017-03-27T16:36:45Z

packages/mdc-ripple/_mixins.scss

-      mdc-ripple-fg-unbounded-transform-deactivate var(--mdc-ripple-fg-unbounded-transform-duration, 0) $mdc-animation-fast-out-slow-in-timing-function;
+  &.mdc-ripple-upgraded--foreground-deactivation#{$pseudo} {
+    // Retain transform from mdc-ripple-fg-radius-in activation
+    transform: translate(var(--mdc-ripple-fg-translate-end, 0)) scale(var(--mdc-ripple-fg-scale, 1));


Same thing here.

yeelan0319 · 2017-03-27T19:23:56Z

@traviskaufman
Brought this up in our stand up today as a parking lot since it seems like there is no further information about why bounded & unbounded is removed and I feel I am lacking of quite a few context information to give useful feedbacks on those implementation.

Moreover since so many changes have been made, I also want to make sure there is no changing to our API and we don't need to make any updates to README.md.

Given we might need to review code across different components and git doesn't always clever enough to show diff correctly, I feel it might be helpful to provide slightly more high level description about the commit, I found a guideline on Github that we might found useful.

traviskaufman · 2017-03-28T14:45:11Z

@yeelan0319

When the ripple was originally designed from a motion perspective, bounded and unbounded activation/deactivation used two different animation styles, distinct from one another.

The UX updates to the ripple remove this distinction of bounded vs. unbounded solely with regard to how the ripple animates in and out. There is still a distinction between bounded vs. unbounded, but we no longer have to account for this when animating the ripple. Thus, there are no API changes; only UX changes.

I'll go ahead and add this description above to the commit message.

traviskaufman · 2017-03-28T14:56:52Z

@yeelan0319 changes made PTAL

…lines for ripple Background === When the ripple was originally designed from a motion perspective, bounded and unbounded activation/deactivation used two different animation styles, distinct from one another. The UX updates to the ripple remove this distinction of bounded vs. unbounded solely with regard to how the ripple animates in and out. There is still a distinction between bounded vs. unbounded, but we no longer have to account for this when animating the ripple. Thus, there are no API changes; only UX changes. Changes === - Implement UX changes for "tap" + "tap and hold" interactions - Remove all references of "bounded" vs. "unbounded" from activation/deactivation code Resolves #190

yeelan0319 · 2017-03-28T16:21:40Z

Cool. Thanks Travis. This feedback is useful 👍 🌟

…lines (material-components#419) Background === When the ripple was originally designed from a motion perspective, bounded and unbounded activation/deactivation used two different animation styles, distinct from one another. The UX updates to the ripple remove this distinction of bounded vs. unbounded solely with regard to how the ripple animates in and out. There is still a distinction between bounded vs. unbounded, but we no longer have to account for this when animating the ripple. Thus, there are no API changes; only UX changes. Changes === - Implement UX changes for "tap" + "tap and hold" interactions - Remove all references of "bounded" vs. "unbounded" from activation/deactivation code Resolves material-components#190

traviskaufman requested review from yeelan0319 and cristobalchao March 21, 2017 17:03

BBosman reviewed Mar 21, 2017

View reviewed changes

traviskaufman force-pushed the feat/implement-improved-interaction-guidelines-new branch from bac68c8 to cfe9686 Compare March 21, 2017 18:44

cristobalchao approved these changes Mar 22, 2017

View reviewed changes

yeelan0319 suggested changes Mar 23, 2017

View reviewed changes

traviskaufman force-pushed the feat/implement-improved-interaction-guidelines-new branch from cfe9686 to ab120d8 Compare March 24, 2017 19:01

yeelan0319 suggested changes Mar 27, 2017

View reviewed changes

traviskaufman force-pushed the feat/implement-improved-interaction-guidelines-new branch from ab120d8 to e32c1fd Compare March 28, 2017 14:56

traviskaufman force-pushed the feat/implement-improved-interaction-guidelines-new branch from e32c1fd to 322f2b6 Compare March 28, 2017 15:23

yeelan0319 approved these changes Mar 28, 2017

View reviewed changes

traviskaufman merged commit 046e337 into master Mar 28, 2017

traviskaufman deleted the feat/implement-improved-interaction-guidelines-new branch March 28, 2017 16:27

amsheehan mentioned this pull request Mar 30, 2017

Ripples not being allowed to complete. #434

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ripple): Implement subset of improved interaction response guidelines #419

feat(ripple): Implement subset of improved interaction response guidelines #419

traviskaufman commented Mar 21, 2017 •

edited

Loading

traviskaufman commented Mar 21, 2017

codecov-io commented Mar 21, 2017 •

edited

Loading

BBosman Mar 21, 2017

BBosman Mar 21, 2017

traviskaufman Mar 21, 2017

BBosman Mar 21, 2017

BBosman Mar 21, 2017

traviskaufman Mar 21, 2017

BBosman Mar 21, 2017

traviskaufman Mar 21, 2017

yeelan0319 Mar 23, 2017

traviskaufman commented Mar 24, 2017

yeelan0319 Mar 27, 2017

traviskaufman Mar 28, 2017

yeelan0319 Mar 27, 2017

traviskaufman Mar 28, 2017

yeelan0319 commented Mar 27, 2017

traviskaufman commented Mar 28, 2017

traviskaufman commented Mar 28, 2017

yeelan0319 commented Mar 28, 2017

feat(ripple): Implement subset of improved interaction response guidelines #419

feat(ripple): Implement subset of improved interaction response guidelines #419

Conversation

traviskaufman commented Mar 21, 2017 • edited Loading

Background

Changes

traviskaufman commented Mar 21, 2017

codecov-io commented Mar 21, 2017 • edited Loading

Codecov Report

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

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

Choose a reason for hiding this comment

traviskaufman commented Mar 24, 2017

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

yeelan0319 commented Mar 27, 2017

traviskaufman commented Mar 28, 2017

traviskaufman commented Mar 28, 2017

yeelan0319 commented Mar 28, 2017

traviskaufman commented Mar 21, 2017 •

edited

Loading

codecov-io commented Mar 21, 2017 •

edited

Loading