feat(cdk-scrollable): add methods to normalize scrolling in RTL #12607

mmalerba · 2018-08-09T20:31:38Z

Technically a major change due to tightening the ElementRef to ElementRef<HTMLElement>

jelbourn · 2018-08-09T21:19:16Z

src/cdk/platform/features.ts

@@ -8,6 +8,7 @@

 /** Cached result of whether the user's browser supports passive event listeners. */
 let supportsPassiveEvents: boolean;
+let rtlScrollAxisType: 'normal' | 'negated' | 'inverted';


Add description?

jelbourn · 2018-08-09T21:21:37Z

src/cdk/platform/features.ts

+ * - inverted: scrollLeft is (scrollWidth - clientWidth) when scrolled all the way left and 0 when
+ *     scrolled all the way right.
+ */
+export function getRtlScrollAxisType(): 'normal' | 'negated' | 'inverted' {


Can you add a comment somewhere in this file like "As of time of this writing, Chrome is ..., Firefox is ..."

jelbourn · 2018-08-09T21:23:23Z

src/cdk/platform/features.ts

@@ -8,6 +8,7 @@

 /** Cached result of whether the user's browser supports passive event listeners. */
 let supportsPassiveEvents: boolean;
+let rtlScrollAxisType: 'normal' | 'negated' | 'inverted';


I think this would be a good time to break features.ts into separate files for different features

jelbourn · 2018-08-09T21:24:36Z

src/cdk/platform/features.ts

@@ -8,6 +8,7 @@

 /** Cached result of whether the user's browser supports passive event listeners. */
 let supportsPassiveEvents: boolean;
+let rtlScrollAxisType: 'normal' | 'negated' | 'inverted';


Could this use an enum? In other places where we use string literal types, it's usually because the value can be provided in a template

jelbourn · 2018-08-09T21:31:15Z

src/cdk/platform/features.ts

+  }
+
+  if (!rtlScrollAxisType) {
+    const viewport = document.createElement('div');


viewport -> scrollingContainer? Since viewport is often used to refer to the whole browser viewport.

jelbourn · 2018-08-09T21:48:39Z

src/cdk/scrolling/scrollable.spec.ts

+class ScrollableViewport {
+  @Input() dir: Direction;
+  @ViewChild(CdkScrollable) scrollable: CdkScrollable;
+  @ViewChild('viewport') viewport: ElementRef<HTMLElement>;


viewport -> scrollingContainer?

jelbourn · 2018-08-09T22:07:54Z

src/cdk/scrolling/scrollable.ts

+   */
+  end?: number;
+  /** A distance relative to the bottom edge of the viewport. */
+  bottom?: number;


So, you could capture the mutual exclusivity with TypeScript. The error message is a little wordy.

(idea stolen from microsoft/TypeScript#14094)

jelbourn · 2018-08-09T22:14:03Z

src/cdk/scrolling/scrollable.ts

  }

-  getElementRef(): ElementRef {
-    return this._elementRef;
+  private _scrollTo(options: ScrollToOptions): void {


Could this have a different name than the public method that captures what it does differently / more?

jelbourn · 2018-08-09T22:15:38Z

src/cdk/scrolling/scrollable.ts

+   * Measures the scroll offset relative to the specified edge of the viewport.
+   * @param from The edge to measure from.
+   */
+  measureScrollOffset(from: 'top' | 'left' | 'right' | 'bottom' | 'start' | 'end'): number {


Could the type for from be an enum, or does it come from something a user would put in a template?

If it can't be an enum, it might be good to alias it.

This one could appear in a template, I'll alias the values used more than once

jelbourn · 2018-08-09T22:22:05Z

src/cdk/scrolling/scrollable.ts

+      from = isRtl ? 'left' : 'right';
+    }
+
+    if (isRtl && getRtlScrollAxisType() == 'inverted') {


I think it would make this code more understandable if you had a brief comment like like

// normal: // inverted: // negated:

Even though that's already written in the feature detection function.

It would also be good to explicitly say in a comment which system you're normalizing to (I assume it's normal).

crisbeto · 2018-08-10T06:08:14Z

src/cdk/platform/features/scrolling.ts

+ */
+
+/** The possible ways the browser may handle the horizontal scroll axis in RTL languages. */
+export enum RtlScrollAxisType {


Didn't we have something about not using enums @jelbourn?

@jelbourn actually requested this, since its not a value that a user would supply through a template

I have no memory about not wanting to use enums (for things that aren't used via templates). It is very possible I forgot something, though.

I have some memory about it messing some internal tooling (Closure maybe?), but I could be mixing things up.

crisbeto · 2018-08-10T06:09:21Z

src/cdk/platform/features/scrolling.ts

+
+/** Check whether the browser supports scroll behaviors. */
+export function supportsScrollBehavior(): boolean {
+  return !!(document && document.documentElement && document.documentElement.style &&


This will throw an error during server-side rendering. The check for document should be typeof document !== 'undefined'. Also you don't need checks for document.documentElement and documentElement.style since those are guaranteed to be there if we have a document.

crisbeto · 2018-08-10T06:10:57Z

src/cdk/platform/features/scrolling.ts

+    scrollContainer.style.overflow = 'auto';
+    scrollContainer.style.visibility = 'hidden';
+    scrollContainer.style.pointerEvents = 'none';
+    scrollContainer.style.position = 'absolute';


Consider having a variable for scrollContainer.style so it doesn't have to be repeated as much. It should minify a bit better as well.

crisbeto · 2018-08-10T06:11:52Z

src/cdk/platform/features/scrolling.ts

+          scrollContainer.scrollLeft == 0 ? RtlScrollAxisType.NEGATED : RtlScrollAxisType.INVERTED;
+    }
+
+    document.body.removeChild(scrollContainer);


This would be a little more future-proof as scrollContainer.parentNode.removeChild(scrollContainer). This would start failing if we started appending the element to something different from the body.

crisbeto · 2018-08-10T06:12:06Z

src/cdk/platform/features/scrolling.ts

+      // return 0 when we read it again.
+      scrollContainer.scrollLeft = 1;
+      rtlScrollAxisType =
+          scrollContainer.scrollLeft == 0 ? RtlScrollAxisType.NEGATED : RtlScrollAxisType.INVERTED;


Should be triple equals.

crisbeto · 2018-08-10T06:14:07Z

src/cdk/scrolling/scrollable.spec.ts

+
+function checkIntersecting(r1: ClientRect, r2: ClientRect, expected = true) {
+  const actual =
+      r1.left < r2.right && r1.right > r2.left && r1.top < r2.bottom && r1.bottom > r2.top;


Should these be <=, >= etc?

No, elements that are side-by-side would be detected as overlapping if we did that

crisbeto · 2018-08-10T06:15:19Z

src/cdk/scrolling/scrollable.spec.ts

+      checkIntersecting(testComponent.scrollContainer.nativeElement.getBoundingClientRect(),
+          testComponent.lastRowEnd.nativeElement.getBoundingClientRect(), false);
+
+      expect(testComponent.scrollable.measureScrollOffset('top')).toBe(0);


Seems like a bunch of long expressions like testComponent.scrollContainer.nativeElement are being repeated a lot. Consider moving them into variables to reduce noise.

crisbeto · 2018-08-10T06:18:59Z

src/cdk/scrolling/scrollable.ts

+ * start, and end properties.
+ */
+export interface ExtendedScrollToOptions extends ScrollToOptions {
+  /** A distance relative to the right edge of the viewport. */


super tiny nit: the property description don't have to start with "a".

mmalerba · 2018-08-10T17:09:31Z

PTAL, comments addressed

crisbeto

LGTM

jelbourn

LGTM aside from one nti

jelbourn · 2018-08-10T18:36:21Z

src/cdk/scrolling/scrollable.ts

+export type _Left = { left?: number };
+export type _Right = { right?: number };
+export type _Start = { start?: number };
+export type _End = { end?: number };


Omit spaces in braces

We could probably make the new noImportSpacing rule also cover the spacing inside of objects. Similar as the object-curly-spacing rule from ESlint. I'll have a quick look at some point.

angular-automatic-lock-bot · 2019-09-09T10:02:24Z

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 added the target: major This PR is targeted for the next major release label Aug 9, 2018

mmalerba assigned jelbourn and crisbeto Aug 9, 2018

mmalerba requested review from jelbourn and crisbeto August 9, 2018 20:31

mmalerba requested review from andrewseguin and devversion as code owners August 9, 2018 20:31

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

mmalerba force-pushed the scrollable-rtl branch from cd90ac9 to 6b5485f Compare August 9, 2018 20:53

jelbourn reviewed Aug 9, 2018

View reviewed changes

crisbeto reviewed Aug 10, 2018

View reviewed changes

mmalerba force-pushed the scrollable-rtl branch 3 times, most recently from b4da004 to e9c384b Compare August 10, 2018 17:09

crisbeto approved these changes Aug 10, 2018

View reviewed changes

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

jelbourn approved these changes Aug 10, 2018

View reviewed changes

mmalerba force-pushed the scrollable-rtl branch from e9c384b to 8dee516 Compare August 10, 2018 18:52

mmalerba added 2 commits August 13, 2018 09:46

feat(cdk-scrollable): add methods to normalize scrolling in RTL

d29378e

address comments

7411d8c

mmalerba force-pushed the scrollable-rtl branch from 8dee516 to 7411d8c Compare August 13, 2018 16:50

mmalerba merged commit 028746a into angular:master Aug 13, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cdk-scrollable): add methods to normalize scrolling in RTL #12607

feat(cdk-scrollable): add methods to normalize scrolling in RTL #12607

mmalerba commented Aug 9, 2018

jelbourn Aug 9, 2018

jelbourn Aug 9, 2018

jelbourn Aug 9, 2018

jelbourn Aug 9, 2018

jelbourn Aug 9, 2018

jelbourn Aug 9, 2018

jelbourn Aug 9, 2018

jelbourn Aug 9, 2018

jelbourn Aug 9, 2018

mmalerba Aug 9, 2018

jelbourn Aug 9, 2018

crisbeto Aug 10, 2018

mmalerba Aug 10, 2018

jelbourn Aug 10, 2018

crisbeto Aug 10, 2018

crisbeto Aug 10, 2018

crisbeto Aug 10, 2018

crisbeto Aug 10, 2018

crisbeto Aug 10, 2018

crisbeto Aug 10, 2018

mmalerba Aug 10, 2018

crisbeto Aug 10, 2018

crisbeto Aug 10, 2018

mmalerba commented Aug 10, 2018

crisbeto left a comment

jelbourn left a comment

jelbourn Aug 10, 2018

devversion Aug 10, 2018 •

edited

Loading

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

feat(cdk-scrollable): add methods to normalize scrolling in RTL #12607

feat(cdk-scrollable): add methods to normalize scrolling in RTL #12607

Conversation

mmalerba commented Aug 9, 2018

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

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

mmalerba commented Aug 10, 2018

crisbeto left a comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devversion Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

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

devversion Aug 10, 2018 •

edited

Loading