feat(layout-direction): a service to accompany the Dir directive #2286

EladBezalel · 2016-12-19T23:04:21Z

Finds the application root element and traverses upwards and looking for dir attribute
If Dir directive is available throughout the application it will be injected instead the activeLayoutDirection service

jelbourn · 2016-12-20T17:32:03Z

src/lib/core/core.ts

@@ -1,6 +1,6 @@
 import {NgModule, ModuleWithProviders} from '@angular/core';
 import {MdLineModule} from './line/line';
-import {RtlModule} from './rtl/dir';
+import {RtlModule} from './rtl/index';


You shouldn't need index in there

jelbourn · 2016-12-20T17:32:32Z

src/lib/core/rtl/active-layout-direction.ts

+
+  constructor(private _appRef: ApplicationRef) {}
+
+  getActiveDirection(injector: Injector): Promise<LayoutDirection> {


Needs JsDoc description (here and for other method)

jelbourn · 2016-12-20T17:33:52Z

src/lib/core/rtl/active-layout-direction.ts

+      })
+      .then(rootComponent => {
+        return this._getParentDirection(rootComponent);
+      });


return Promise.resolve(null) .then(() => this._getRootElement(injector)) .then(rootComponent => this._getParentDirection(rootComponent));

Also add comment explaining why the Promise.resolve is necessary

jelbourn · 2016-12-20T17:34:36Z

src/lib/core/rtl/active-layout-direction.ts

+
+    for (let i = 0; i < this._appRef.componentTypes.length; i++) {
+      if (injector.get(this._appRef.componentTypes[i])) {
+        rootComponent = this._appRef.components[i].location;


You can just return directly here

jelbourn · 2016-12-20T17:41:53Z

src/lib/core/rtl/active-layout-direction.ts

+
+@Injectable()
+export class ActiveLayoutDirection {
+  dirChange = new EventEmitter<void>();


Can do someting like

dirChange = Observable.of([])

With a comment explaining that this won't ever fire anything

jelbourn · 2016-12-20T17:52:44Z

src/lib/core/rtl/active-layout-direction.ts

+} from '@angular/core';
+
+import {LayoutDirection} from './dir';
+import {EventEmitter} from '@angular/common/src/facade/async';


This is unnecessary since it will be changed to an Observable, but FYI never deep import from anything in @angular/xxx. This would normally be

import {EventEmitter} from '@angular/core';

Webstorm auto import 😅

jelbourn · 2016-12-20T17:54:22Z

src/lib/core/rtl/active-layout-direction.ts

+    return rootComponent.nativeElement;
+  }
+
+  _getParentDirection(element: HTMLElement): string {


_getClosestAncestorDirValue

jelbourn · 2016-12-20T17:56:18Z

src/lib/grid-list/grid-list.ts

  constructor(
      private _renderer: Renderer,
      private _element: ElementRef,
-      @Optional() private _dir: Dir) {}
+      @Optional() _activeLayoutDirection: ActiveLayoutDirection,
+      @Optional() _injector: Injector) {


Injector doesn't have to be @Optional. You can always inject an Injector.

jelbourn · 2016-12-20T18:04:40Z

src/lib/core/rtl/active-layout-direction.ts

+import {EventEmitter} from '@angular/common/src/facade/async';
+
+@Injectable()
+export class ActiveLayoutDirection {


Rename this class to something like GlobalDirAccessor and then add another class called ActiveLayoutDirection that just looks like

export class ActiveLayoutDirection { dirChange: Observable<void>; getActiveDirection(injector: Injector): Promise<LayoutDirection> { return null; } }

The both Dir and GlobalDirAccessor will have implements ActiveLayoutDirection (TypeScript lets you implement a class just like an interface. Keeping it as a class lets us use it as an injection token).

- Finds the application root element and traverses upwards and looking for dir attribute - If Dir directive is available throughout the application it will be injected instead the activeLayoutDirection service fixes angular#1719

jelbourn · 2017-01-13T01:43:12Z

@EladBezalel was this ready for another round of review?

EladBezalel · 2017-01-13T05:14:25Z

nope 😅

EladBezalel · 2017-03-14T18:19:22Z

A better solution was suggested on #3600

angular-automatic-lock-bot · 2019-09-06T10:01:36Z

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

EladBezalel requested a review from jelbourn December 19, 2016 23:04

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

jelbourn reviewed Dec 20, 2016

View reviewed changes

EladBezalel force-pushed the feat/layout-direction branch from fd9b073 to 0e77ce7 Compare December 26, 2016 21:44

EladBezalel force-pushed the feat/layout-direction branch from 0e77ce7 to 33decab Compare January 12, 2017 09:27

EladBezalel force-pushed the feat/layout-direction branch from 33decab to d2e7d3e Compare January 12, 2017 09:59

jelbourn mentioned this pull request Jan 13, 2017

Support injection of ElementRef for root element angular/angular#13919

Closed

andrewseguin added the pr: needs rebase label Mar 2, 2017

EladBezalel closed this Mar 14, 2017

EladBezalel deleted the feat/layout-direction branch March 14, 2017 18:19

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

feat(layout-direction): a service to accompany the Dir directive #2286

feat(layout-direction): a service to accompany the Dir directive #2286

EladBezalel commented Dec 19, 2016

jelbourn Dec 20, 2016

jelbourn Dec 20, 2016

jelbourn Dec 20, 2016

jelbourn Dec 20, 2016

jelbourn Dec 20, 2016

jelbourn Dec 20, 2016

EladBezalel Dec 20, 2016

jelbourn Dec 20, 2016

jelbourn Dec 20, 2016

jelbourn Dec 20, 2016

jelbourn commented Jan 13, 2017

EladBezalel commented Jan 13, 2017

EladBezalel commented Mar 14, 2017

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


		constructor(private _appRef: ApplicationRef) {}

		getActiveDirection(injector: Injector): Promise<LayoutDirection> {

feat(layout-direction): a service to accompany the Dir directive #2286

feat(layout-direction): a service to accompany the Dir directive #2286

Conversation

EladBezalel commented Dec 19, 2016

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

jelbourn commented Jan 13, 2017

EladBezalel commented Jan 13, 2017

EladBezalel commented Mar 14, 2017

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