wip: fix(virtual-scroll): make horizontal scrolling work in RTL #12512

mmalerba · 2018-08-03T17:16:36Z

No description provided.

crisbeto · 2018-08-03T17:20:02Z

src/cdk/scrolling/scrolling-module.ts

@@ -14,8 +15,9 @@ import {CdkVirtualForOf} from './virtual-for-of';
 import {CdkVirtualScrollViewport} from './virtual-scroll-viewport';

 @NgModule({
-  imports: [PlatformModule],
+  imports: [BidiModule, PlatformModule],


I don't think you need to import the BidiModule since the Directionality is provided at the root.

I think it definitely needs to be in exports, so that we detect native dir="rtl" attrs as directives. I'm not sure if it needs to be in imports, I see a lot of other modules doing it though. @jelbourn do you know if having it in imports is still necessary?

It should still be in the imports; I think it only ever worked exclusively in exports unintentionally

crisbeto · 2018-08-03T17:22:11Z

src/cdk/scrolling/virtual-scroll-viewport.spec.ts

+  @Input() itemSize = 50;
+  @Input() bufferSize = 0;
+  @Input() items = Array(10).fill(0).map((_, i) => i);
+  @Input() trackBy;


This one's missing a type.

crisbeto · 2018-08-03T17:24:07Z

src/cdk/scrolling/virtual-scroll-viewport.ts

   * @param offset The offset to scroll to.
   * @param behavior The ScrollBehavior to use when scrolling. Default is behavior is `auto`.
   */
  scrollToOffset(offset: number, behavior: ScrollBehavior = 'auto') {
    const viewportElement = this.elementRef.nativeElement;

+    // For a horizontal viewport in a right-to-left language we need to calculate what `scrollRight`
+    // would be.
+    offset = this.orientation == 'horizontal' && this._dir && this._dir.value == 'rtl' ?


It looks like you're repeating Math.max(0, this._totalContentSize - this._viewportSize - offset) at least in a couple of places. Consider moving it into a helper.

crisbeto · 2018-08-03T17:24:38Z

src/demo-app/demo-app/demo-app.html

@@ -41,7 +41,7 @@ <h1>Angular Material Demos</h1>
      </div>
    </mat-toolbar>

-    <div #root="dir" dir="ltr" class="demo-content">
+    <div #root="dir" dir="rtl" class="demo-content">


Should this be reverted?

jelbourn · 2018-08-07T23:24:03Z

src/cdk/scrolling/virtual-scroll-viewport.ts

+    // For a horizontal viewport in a right-to-left language we need to translate along the x-axis
+    // in the negative direction.
+    const axisDirection = this.orientation == 'horizontal' && this._dir && this._dir.value == 'rtl'
+        ? -1 : 1;


Since you check them a couple of times, I would add _isHorizontal and _isRtl methods. Then this would just look like

// For a horizontal viewport in a right-to-left language we need to translate along the x-axis // in the negative direction. const axisCoefficient = this._isHorizontal && this._isRtl() ? -1 : 1;

ngbot · 2018-08-13T18:04:58Z

Hi @mmalerba! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

mmalerba · 2018-08-21T21:56:23Z

@jelbourn @crisbeto PTAL, I refactored this to have the CdkVirtualScrollViewport extend CdkScrollable and take advantage of the RTL logic I already added to CdkScrollable

crisbeto

LGTM

crisbeto · 2018-08-22T17:18:04Z

src/cdk/scrolling/virtual-scroll-viewport.scss

-  will-change: transform;
+
+  // Note: We can't put `will-change: transform;` here because it causes Safari to not update the
+  // viewport's `scrollHeight` when the spacer's transform changes.


nit: might sound a little weird, but we could even have a unit test for this if we want to be super sure that it never gets reintroduced. E.g. the test would just be expect(getComputedStyle(element).willChange).toBe('none').

I actually did add some unit tests that verify the scrollHeight / scrollWidth are correct after updating the spacer transform. They would fail if someone tried to add this back

crisbeto · 2018-08-22T17:22:44Z

src/cdk/scrolling/scrollable.ts

-              @Optional() private _dir?: Directionality) {}
+  constructor(protected elementRef: ElementRef<HTMLElement>,
+              protected scrollDispatcher: ScrollDispatcher,
+              protected ngZone: NgZone,


Shouldn't all of the protected properties be underscored?

I think we want people to extend their own components from this if they act as a viewport. Therefore the protected properties are kind of a part of the public API and should not be underscored

jelbourn

LGTM aside from Kristiyan's comments

jelbourn · 2018-08-22T18:58:00Z

src/cdk/scrolling/virtual-scroll-viewport.ts

+    // For a horizontal viewport in a right-to-left language we need to translate along the x-axis
+    // in the negative direction.
+    const axisDirection = this.orientation == 'horizontal' && this.dir && this.dir.value == 'rtl'
+        ? -1 : 1;


How about

const isHorizontal = this.orientation == 'horizontal'; const isRtl = this.dir && this.dir.value == 'rtl'; const axisDirection = isHorizontal && isRtl ? -1 : 1;

angular-automatic-lock-bot · 2019-09-09T11:03: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.}

mmalerba assigned jelbourn and crisbeto Aug 3, 2018

mmalerba requested review from jelbourn and crisbeto August 3, 2018 17:16

mmalerba requested a review from andrewseguin as a code owner August 3, 2018 17:16

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 3, 2018

mmalerba force-pushed the vs-rtl branch from 66b9c01 to 8c98bff Compare August 3, 2018 17:18

mmalerba added the target: major This PR is targeted for the next major release label Aug 3, 2018

crisbeto reviewed Aug 3, 2018

View reviewed changes

jelbourn reviewed Aug 7, 2018

View reviewed changes

mmalerba requested a review from devversion as a code owner August 9, 2018 01:13

mmalerba changed the title ~~fix(virtual-scroll): make horizontal scrolling work in RTL~~ wip: fix(virtual-scroll): make horizontal scrolling work in RTL Aug 9, 2018

mmalerba force-pushed the vs-rtl branch 3 times, most recently from 5fb3b9a to dd7cc5f Compare August 14, 2018 18:52

mmalerba added 5 commits August 20, 2018 15:12

fix(virtual-scroll): make horizontal scrolling work in RTL

7da621d

address comments

4c25e86

wip: scrollable

d2ce70d

fix rebase

df672ee

refactor to use CdkScrollable

cb132c8

mmalerba force-pushed the vs-rtl branch from dd7cc5f to cb132c8 Compare August 20, 2018 22:13

mmalerba added 2 commits August 20, 2018 16:01

fix lint and tests

d9fa498

fix safari

6aedbcf

mmalerba force-pushed the vs-rtl branch from e9184d3 to 6aedbcf Compare August 21, 2018 21:05

crisbeto approved these changes Aug 22, 2018

View reviewed changes

crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Aug 22, 2018

jelbourn approved these changes Aug 22, 2018

View reviewed changes

address comments

c018e6f

jelbourn merged commit 22953a9 into angular:master Aug 23, 2018

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

wip: fix(virtual-scroll): make horizontal scrolling work in RTL #12512

wip: fix(virtual-scroll): make horizontal scrolling work in RTL #12512

Uh oh!

Conversation

mmalerba commented Aug 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngbot bot commented Aug 13, 2018

Uh oh!

mmalerba commented Aug 21, 2018

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

jelbourn Aug 7, 2018 •

edited

Loading