fix(ripple): make ripples conform with specs #2859

devversion · 2017-01-29T19:39:32Z

This makes the ripple service conform with the specifications and other Material reference implementations.

See https://material.io/guidelines/motion/material-motion.html#material-motion-how-does-material-move

Reference from the Material specifications

Ink ripples confirm user input by immediately expanding outward from the point of touch. The card lifts to indicate an active state.

This means the following:

Ripples now trigger on mousedown
Ripples now persists as long as the element is being hold.
No longer adds an unnecessary background ripple.
Removes the ugly scale(0.00001) for ripple animations

Not only visually the ripple has been changed. The whole renderer has been rewritten and now has a very simple API, that can be easily used by developers.

Also the API has been changed to something more intuitive

// (pageX, pageY, config)
ripple.createRipple(0, 0, {
  centered: true, 
  speedFactor: 0.1
});

As seen in the example, developers are now able to specify a simple Ripple Config object

Live Demo: https://material2-dashboard.firebaseapp.com/button (temporary)

References #1434

devversion · 2017-01-30T10:43:47Z

@fxck created a comment which questioned the dimming of the Material 1 buttons

That's because Material 1 sets an option to dim the background (see). This isn't part of the specification as seen here and shouldn't be a big deal either.

jelbourn

Very nice, overall very clean and much simpler tests

jelbourn · 2017-01-31T01:39:03Z

src/lib/core/ripple/ripple-renderer.ts

-  private _rippleElement: HTMLElement;
+
+  /** Element where the ripples are being added to. */
+  private _targetElement: HTMLElement;


How about we call this containerElement?

jelbourn · 2017-01-31T01:41:21Z

src/lib/core/ripple/ripple-renderer.ts

+  private _isMousedown: boolean = false;
+
+  /** Currently showing ripples that will be closed on mouseup. */
+  private _showingRipples: HTMLElement[] = [];


_activeRipples?

jelbourn · 2017-01-31T02:31:04Z

src/lib/core/ripple/ripple.ts

-  @Input('md-ripple-trigger')
-  get _triggerDeprecated() { return this.trigger; }
-  set _triggerDeprecated(value: HTMLElement|HTMLElement) { this.trigger = value; };
+  @Input('mdRippleTrigger') trigger: any;


I'd rather keep the type on this, even if it is clunky

(the HTMLElement|HTMLElement thing is a workaround for server-side prerendering)

jelbourn · 2017-01-31T02:31:54Z

src/lib/core/ripple/ripple.ts

-        isKeyEvent);
-    }
+  /** Creates a manual ripple at the specified position. */
+  createRipple(pageX: number, pageY: number, config?: RippleConfig) {


Since this is already the ripple class, we could just call this something like launch()

jelbourn · 2017-01-31T03:01:47Z

src/lib/core/ripple/ripple-renderer.ts

+    // Specify events which need to be registered on the trigger.
+    this._triggerEvents.set('mousedown', this.onMousedown.bind(this));
+    this._triggerEvents.set('mouseup', this.onMouseup.bind(this));
+    this._triggerEvents.set('mouseleave', this.onMouseLeave.bind(this));


Rather than using a Map, how about an array of tuples? Better for iteration

let triggerEvents: [string, (MouseEvent) => void][] = [ ['mousedown', e => this.onMouseDown(e)], // ... ];

(generally prefer arrow functions to .bind())

Don't think that will be better for iteration. A map already provides a suitable iterator for those eventpairs.

I would personally like to stay with the Map for convenience and readability.

devversion · 2017-01-31T15:58:38Z

@jelbourn Addressed your feedback. Thanks.

mmalerba

<3 this is amazing

mmalerba · 2017-02-02T16:31:43Z

src/lib/checkbox/_checkbox-theme.scss

@@ -68,16 +68,16 @@
    }
  }

-  .md-checkbox:not(.md-checkbox-disabled) {
-    &.md-primary .md-checkbox-ripple .md-ripple-foreground {
+  md-checkbox:not(.md-checkbox-disabled) {


use class rather than element selector (so it works with comparability mode mat-* elements as well)

Good point. I actually had that change only in this PR for testing purposes. I have another PR open for this. See #2857

Also I think we can only use the class here, as soon as Andrew's PR is merged.

mmalerba · 2017-02-02T16:35:04Z

src/lib/checkbox/checkbox.html

@@ -18,8 +18,7 @@
    <div md-ripple *ngIf="!_isRippleDisabled()" class="md-checkbox-ripple"
         [mdRippleTrigger]="_getHostElement()"
         [mdRippleCentered]="true"
-         [mdRippleSpeedFactor]="0.3"
-         mdRippleBackgroundColor="rgba(0, 0, 0, 0)"></div>
+         [mdRippleSpeedFactor]="0.3"></div>


does the spec say anything about what the speed of these ripples should be? i think the button one looks a little slower in your demo than it does in the specs

As discussed on Slack, we are going to have another PR to make all component ripples align with the specs.

created #2901 for this

Should I assign it to me?

mmalerba · 2017-02-02T16:52:47Z

src/lib/core/ripple/ripple-renderer.ts

+    let offsetX = pageX - containerRect.left;
+    let offsetY = pageY - containerRect.top;
+
+    let ripple = document.createElement('div');


can we use the Renderer to create this and set styles?

As discussed on Slack, we will revisit this in issue #308 and once we have feedback from Jeremy about this.

mmalerba · 2017-02-02T16:57:17Z

src/lib/core/ripple/ripple-renderer.ts

+    // ripple elements. This is critical because then the `scale` would not animate properly.
+    // Enforce a style recalculation by calling `getComputedStyle` and accessing any property.
+    // See: https://gist.github.com/paulirish/5d52fb081b3570c81e3a
+    window.getComputedStyle(ripple).getPropertyValue('opacity');


this seems like a good candidate for extracting into some DOM utility class, I don't think we have one yet, but maybe make it a private method with a note that we can extract it?

mmalerba · 2017-02-02T16:57:49Z

src/lib/core/ripple/ripple-renderer.ts

+
+    ripple.style.transform = 'scale(1)';
+
+    // Wait for the ripple to be faded in. Once faded in the ripple can be hided if the mouse is


s/hided/hidden

mmalerba · 2017-02-02T17:02:48Z

src/lib/core/ripple/ripple.spec.ts

@@ -86,112 +65,86 @@ describe('MdRipple', () => {
    document.body.style.margin = originalBodyMargin;
  });

+  function dispatchMouseEvent(type: string, offsetX = 0, offsetY = 0) {


I swear this function is in like half of our test files, we should create a test utilities file sometime (not part of this PR though)

created #2902 for this

mmalerba · 2017-02-02T17:07:03Z

src/lib/radio/_radio-theme.scss

@@ -29,7 +29,7 @@
    }
  }

-  .md-radio-ripple .md-ripple-foreground {
+  .md-radio-ripple .md-ripple-element {


when adding new classes make them mat-* to make andrew's life easier :p

devversion · 2017-02-02T18:47:34Z

@mmalerba Thanks for your feedback. Addressed the comments you added. Also glad you like it :)

mmalerba · 2017-02-02T19:29:38Z

lgtm

tinayuangao

LGTM

tinayuangao · 2017-02-03T22:29:05Z

src/lib/core/ripple/ripple-renderer.ts

-const MIN_RIPPLE_FILL_TIME_SECONDS = 0.1;
-const MAX_RIPPLE_FILL_TIME_SECONDS = 0.3;
+export const RIPPLE_SPEED_PX_PER_SECOND = 170;
+export const RIPPLE_FADE_OUT_DURATION = 600;


Add doc somewhere to mention the fade out duration is fixed and is not changed with speed factor?

Good point. Added it to the @Input() and also added comments for those constants.

This makes the ripple service conform with the specifications and other Material reference implementations. See https://material.io/guidelines/motion/material-motion.html#material-motion-how-does-material-move This means the following: * Ripples now trigger on `mousedown` * Ripples now persists as long as the element is being hold. * No longer adds an unnecessary background ripple. * Removes the ugly `scale(0.00001)` for ripple animations Not only visually the ripple has been changed. The whole renderer has been rewritten and now has a very simple API, that can be easily used by developers. References angular#1434

…ctor

devversion · 2017-02-09T17:00:02Z

Just rebased the PR due to the compatibility changes.

abdel-ships-it · 2017-02-13T09:54:18Z

Great PR! Was about to open up an issue about the current state of ripple effects.

angular-automatic-lock-bot · 2019-09-06T08:02:29Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

devversion added the pr: needs review label Jan 29, 2017

devversion assigned jelbourn Jan 29, 2017

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 29, 2017

devversion force-pushed the feat/ripple-specs branch from ae80099 to 22d09ee Compare January 30, 2017 18:08

jelbourn requested a review from tinayuangao January 30, 2017 23:51

jelbourn reviewed Jan 31, 2017

View reviewed changes

devversion force-pushed the feat/ripple-specs branch from 59cfd11 to 6911161 Compare January 31, 2017 15:58

devversion force-pushed the feat/ripple-specs branch from 6911161 to f77b5cd Compare January 31, 2017 17:50

devversion requested a review from mmalerba February 2, 2017 15:31

mmalerba reviewed Feb 2, 2017

View reviewed changes

devversion mentioned this pull request Feb 2, 2017

slide-toggle: use ripple service instead of temporary ripple #2900

Closed

devversion force-pushed the feat/ripple-specs branch from a32ea4f to b48d2a9 Compare February 3, 2017 10:20

tinayuangao reviewed Feb 3, 2017

View reviewed changes

mmalerba added pr: lgtm and removed pr: needs review labels Feb 3, 2017

devversion added the action: merge The PR is ready for merge by the caretaker label Feb 4, 2017

devversion added 5 commits February 9, 2017 17:15

Fix linting and IE11 unsupported error

8566532

Ensure style recalculation

77c46bb

Address comments

af169a0

Address feedback

f972123

devversion force-pushed the feat/ripple-specs branch from 630cc8c to 209e4b0 Compare February 9, 2017 16:48

Document that fade-out duration can't be modified through the speedFa…

48bccb5

…ctor

devversion force-pushed the feat/ripple-specs branch from 209e4b0 to 48bccb5 Compare February 9, 2017 16:55

tinayuangao merged commit 6381948 into angular:master Feb 10, 2017

devversion deleted the feat/ripple-specs branch February 10, 2017 05:53

devversion mentioned this pull request Feb 15, 2017

Ripple speed too fast, and a bit too faded #2626

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ripple): make ripples conform with specs #2859

fix(ripple): make ripples conform with specs #2859

devversion commented Jan 29, 2017 •

edited

Loading

devversion commented Jan 30, 2017 •

edited

Loading

jelbourn left a comment

jelbourn Jan 31, 2017

jelbourn Jan 31, 2017

jelbourn Jan 31, 2017

jelbourn Jan 31, 2017

jelbourn Jan 31, 2017

devversion Jan 31, 2017

devversion commented Jan 31, 2017

mmalerba left a comment

mmalerba Feb 2, 2017

devversion Feb 2, 2017

mmalerba Feb 2, 2017

devversion Feb 2, 2017

mmalerba Feb 2, 2017

devversion Feb 2, 2017

mmalerba Feb 2, 2017

devversion Feb 2, 2017

mmalerba Feb 2, 2017

mmalerba Feb 2, 2017

mmalerba Feb 2, 2017

mmalerba Feb 2, 2017

mmalerba Feb 2, 2017

devversion commented Feb 2, 2017

mmalerba commented Feb 2, 2017

tinayuangao left a comment

tinayuangao Feb 3, 2017

devversion Feb 4, 2017

devversion commented Feb 9, 2017

abdel-ships-it commented Feb 13, 2017

angular-automatic-lock-bot bot commented Sep 6, 2019


		ripple.style.transform = 'scale(1)';

		// Wait for the ripple to be faded in. Once faded in the ripple can be hided if the mouse is

fix(ripple): make ripples conform with specs #2859

fix(ripple): make ripples conform with specs #2859

Conversation

devversion commented Jan 29, 2017 • edited Loading

devversion commented Jan 30, 2017 • edited Loading

jelbourn left a 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

devversion commented Jan 31, 2017

mmalerba left a 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

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

devversion commented Feb 2, 2017

mmalerba commented Feb 2, 2017

tinayuangao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devversion commented Feb 9, 2017

abdel-ships-it commented Feb 13, 2017

angular-automatic-lock-bot bot commented Sep 6, 2019

devversion commented Jan 29, 2017 •

edited

Loading

devversion commented Jan 30, 2017 •

edited

Loading