fix(ripple): Fix the ripple position #1907

tinayuangao · 2016-11-17T04:27:06Z

When scroll down the page, the ripple position is off
Closes #1817

Original PR #1820 is accidentally closed.

jelbourn · 2016-11-17T21:48:53Z

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

+      let top = 75;
+
+      // Add a very large element to make the page scroll
+      let veryLargeElement: HTMLElement = document.createElement('div');


You don't need the type here; document.createElement('div'); will return an HTMLDivElement

(typescript understands string literals in its type system so it can know things like document.createElement or addEventListener can have different arguments based on the string based to them)

jelbourn · 2016-11-17T21:52:20Z

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

+      document.body.scrollLeft = pageScrollLeft;
+
+      rippleElement.style.left = `${elementLeft}px`;
+      rippleElement.style.top = `${elementTop}px`;


Setting the left and top of the rippleElement probably won't do what you want if the element isn't position: absolute.

I'm not sure if the rippleElement is in the DOM; it would have to be for the positioning to be right.

This might be why the tests are failing.

jelbourn · 2016-11-17T22:00:41Z

src/lib/core/ripple/ripple.ts

+          document.body.scrollTop || 0;
+      var scrollLeft = window.pageXOffset || document.documentElement.scrollLeft ||
+          document.body.scrollLeft || 0;
+      this.end(event.pageX - scrollLeft, event.pageY - scrollTop, isKeyEvent);


Can you use ViewportRuler.getViewportScrollPosition for this?

tinayuangao · 2016-11-18T20:00:53Z

Tests fixed. PTAL. Thanks!

jelbourn · 2016-11-28T22:57:40Z

@tinayuangao can you rebase this PR?

tinayuangao · 2016-12-02T22:22:56Z

Rebased and fixed the test.

jelbourn · 2016-12-02T22:34:28Z

src/lib/core/overlay/position/viewport-ruler.ts

-        -documentRect.left :
-        document.body.scrollLeft;
+    const top = -documentRect.top || document.body.scrollTop || window.scrollY || 0;
+    const left = -documentRect.left || document.body.scrollLeft || window.scrollX || 0;


What was your reason for changing this?

The mobile safari/IE test will fail without the window.scrollX/Y case.

jelbourn · 2016-12-02T22:36:16Z

src/lib/button/button.spec.ts

  }
 }
+
+class FakeViewportRuler {


Can we move this to its own file instead of duplicating it? Having fake-viewport-ruler.ts under overlay/position would work.

jelbourn · 2016-12-06T17:30:57Z

LGTM

mmalerba · 2016-12-07T01:19:19Z

Please add ViewportRuler to the forRoot() providers of all components that use MdRipple.

tinayuangao · 2016-12-07T03:05:35Z

Done. Added ViewportRuler to the forRoot() providers.

jelbourn · 2016-12-07T22:16:24Z

Tested this on IE11, Mobile Safari (9), and Firefox and everything seemed good.

jelbourn · 2016-12-07T22:37:49Z

@mmalerba I rebased this and updated the PR

angular-automatic-lock-bot · 2019-09-06T05:01:40Z

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.}

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 17, 2016

tinayuangao added the pr: needs review label Nov 17, 2016

jelbourn reviewed Nov 17, 2016

View reviewed changes

tinayuangao force-pushed the ripple-fix branch 3 times, most recently from 955dd77 to 5419d13 Compare November 18, 2016 19:48

tinayuangao force-pushed the ripple-fix branch from 5419d13 to 243d086 Compare November 28, 2016 23:26

jelbourn mentioned this pull request Nov 30, 2016

md-raised-button on anchor always have disabled styles #1946

Closed

tinayuangao force-pushed the ripple-fix branch 2 times, most recently from c8dc081 to 53b4f6c Compare December 2, 2016 22:18

jelbourn reviewed Dec 2, 2016

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Dec 6, 2016

mmalerba removed the action: merge The PR is ready for merge by the caretaker label Dec 7, 2016

tinayuangao force-pushed the ripple-fix branch 2 times, most recently from 68dfd0e to ea88d34 Compare December 7, 2016 02:50

tinayuangao force-pushed the ripple-fix branch from 32bcde6 to b01a0d1 Compare December 7, 2016 03:17

tinayuangao added the action: merge The PR is ready for merge by the caretaker label Dec 7, 2016

mmalerba added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Dec 7, 2016

tinayuangao added 2 commits December 7, 2016 14:22

Add test. Use viewport ruler

819aba7

fix lint

91b2117

tinayuangao added 4 commits December 7, 2016 14:22

Add ViewportRuler to providers

4cbce97

.

57c8674

.

d83d642

.

95fb0cb

jelbourn force-pushed the ripple-fix branch from b01a0d1 to 95fb0cb Compare December 7, 2016 22:26

jelbourn added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Dec 7, 2016

mmalerba merged commit dd508ea into angular:master Dec 8, 2016

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

fix(ripple): Fix the ripple position #1907

fix(ripple): Fix the ripple position #1907

Uh oh!

Conversation

tinayuangao commented Nov 17, 2016

Uh oh!

jelbourn Nov 17, 2016

Choose a reason for hiding this comment

Uh oh!

jelbourn Nov 17, 2016

Choose a reason for hiding this comment

Uh oh!

jelbourn Nov 17, 2016

Choose a reason for hiding this comment

Uh oh!

tinayuangao commented Nov 18, 2016

Uh oh!

jelbourn commented Nov 28, 2016

Uh oh!

tinayuangao commented Dec 2, 2016

Uh oh!

jelbourn Dec 2, 2016

Choose a reason for hiding this comment

Uh oh!

tinayuangao Dec 6, 2016

Choose a reason for hiding this comment

Uh oh!

jelbourn Dec 2, 2016

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Dec 6, 2016

Uh oh!

mmalerba commented Dec 7, 2016

Uh oh!

tinayuangao commented Dec 7, 2016

Uh oh!

jelbourn commented Dec 7, 2016

Uh oh!

jelbourn commented Dec 7, 2016

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants