feat(directionality): a provider to get the overall directionality #4044

EladBezalel · 2017-04-11T23:37:11Z

Looks at the html and body elements for dir attribute and sets the Directionality service value to it
Whenever someone would try to inject Directionality - if there's a Dir directive up the dom tree it would be provided

jelbourn · 2017-04-12T23:49:42Z

src/lib/core/rtl/dir.ts

 } from '@angular/core';

+import {Directionality} from './directionality';
+export {Directionality} from './directionality';


Can you change the directory name from rtl to bidi and add an index.d.ts that re-exports the public symbols?
We should also rename the NgModule to BidiModule

jelbourn · 2017-04-12T23:50:51Z

src/lib/core/rtl/dir.ts

+import {Directionality} from './directionality';
+export {Directionality} from './directionality';
+
 export type LayoutDirection = 'ltr' | 'rtl';


What do you think of renaming LayoutDirection to just Direction now?

Makes sense

jelbourn · 2017-04-12T23:55:07Z

src/lib/core/rtl/dir.ts

    return {
      ngModule: RtlModule,
-      providers: []
+      providers: [Directionality]


Can you make a provider for Directionality similar to LiveAnnouncer? Something like

export const DIRECTIONALITY_PROVIDER = { // If there is already a Directionality available, use that. Otherwise, provide a new one. provide: Directionality, deps: [[new Optional(), new SkipSelf(), Directionality]], useFactory: function(parentDirectionality) { return parentDirectionality || new Directionality(); } };

Has to use the function syntax rather than () => due to an open bug in the metadata extractor
(should live in directionality.ts)

jelbourn · 2017-04-12T23:55:59Z

src/lib/core/rtl/directionality.ts

+ */
+@Injectable()
+export class Directionality {
+  value: LayoutDirection;


The type def for LayoutDirection should move to this file

jelbourn · 2017-04-13T00:03:07Z

src/lib/core/rtl/directionality.ts

+import {LayoutDirection} from './dir';
+
+/**
+ * A provider to get the direction outside of angular on the `html` or `body` elements


"A provider" isn't really accurate (it's just a class that's provided). It can just be something like

The directionality (LTR / RTL) context for the application (or a subtree of it). Exposes the current direction and a stream of direction changes.

jelbourn · 2017-04-13T00:04:57Z

src/lib/core/rtl/directionality.ts

+
+  constructor() {
+    this.value =
+      (document.body.getAttribute('dir') ||


We still need to account for dir="auto". It looks like HTMLElemenet.dir is also "auto" when that's set to the attribute, but getComputedStyle return either "ltr" or "rtl". I'd like to avoid using getComputedStyle here, though, since we're already calling it for the theming check.

Let's just add a TODO to handle auto with this background info.

jelbourn · 2017-04-13T00:11:21Z

src/lib/core/rtl/directionality.ts

+
+  constructor() {
+    this.value =
+      (document.body.getAttribute('dir') ||


This should also check for the existence of document before dereferencing it (for platform-server)

jelbourn · 2017-04-20T15:05:27Z

@EladBezalel ready for another pass?

EladBezalel · 2017-04-20T15:06:47Z

Yep

jelbourn

Looks good, just a couple last things to address

jelbourn · 2017-04-20T15:08:07Z

src/lib/core/bidi/directionality.ts

+  public change = new EventEmitter<void>();
+
+  constructor() {
+    if (document) {


I think you need typeof document here in order to not error on Angular Universal. This will need to eventually be augmented with using universal's document service.

jelbourn · 2017-04-20T15:18:49Z

src/lib/core/bidi/directionality.ts

+    // but getComputedStyle return either "ltr" or "rtl". avoiding getComputedStyle for now
+    // though, we're already calling it for the theming check.
+    this.value =
+      (document.body.getAttribute('dir') ||


Use the dir property instead of getAttribute? I believe they should be equivalent (but would be shorter)

jelbourn · 2017-04-20T15:26:03Z

src/lib/core/bidi/directionality.ts

+@Injectable()
+export class Directionality {
+  value: Direction = 'ltr';
+  public change = new EventEmitter<void>();


Need unit tests for the following (creating a new bidi.spec.ts):

Directionality reads dir from the html element if not specified on the body

Directionality reads dir from the body even it is also specified on the html element

Directionality defaults to ltr if nothing is specified on either body or the html element

Dir provides itself as Directionality

Dir emits a change event when the value changes

How before the tests i can add the dir attribute to the html and the body elements?

In the top-level beforeEach grab the current dir on each and then in the top-level afterEach restore it. Then you can just change it for individual blocks.

jelbourn · 2017-06-06T00:12:05Z

src/lib/core/bidi/directionality.spec.ts

+function initDir () {
+  document.documentElement.dir = '';
+  document.body.dir = '';
+}


Call this something like clearDocumentDirAttributes? Also move to the bottom of the file, after the tests but before the test components

jelbourn · 2017-06-06T00:13:03Z

src/lib/core/bidi/directionality.spec.ts

+
+    TestBed.compileComponents();
+
+    initDir();


There needs to be an afterEach that restores the dir attributes to whatever they were before.

jelbourn · 2017-06-06T00:14:14Z

src/lib/core/bidi/directionality.spec.ts

+
+/** Test component without Dir. */
+@Component({
+  selector: 'test-app-without-dir',


You don't need a selector for components instantiated via TestBed

jelbourn · 2017-06-06T00:15:50Z

src/lib/core/bidi/directionality.ts

+  public change = new EventEmitter<void>();
+
+  constructor() {
+    if (typeof document) {


typeof document === 'object' && !!document.

We have an isBrowser property on Platform now for things like this, but we can adjust that later (since it involves bringing in PlatformModule)

jelbourn · 2017-06-06T00:20:02Z

src/lib/core/bidi/directionality.spec.ts

+  selector: 'dir-test',
+  template: `<div></div>`
+})
+class DirTest {


Call this InjectsDirectionality with selector injects-directionality. You can also completely replace TestAppWithoutDir with this component.

jelbourn · 2017-06-06T00:21:59Z

src/lib/core/bidi/directionality.spec.ts

+    </div>
+  `
+})
+class TestAppWithDir {


Can just call this ElementWithDir

jelbourn · 2017-06-06T00:26:34Z

src/lib/core/bidi/directionality.spec.ts

+  });
+});
+
+describe('Dir directive', () => {


There should be one top-level describe block that does configureTestingModule, importing BidiModule. You can then have two sub describe blocks ("without dir directive", "with dir directive"). Then you can have a separate beforeEach that does the createComponent in each and the query for the directive you're looking for (to reduce repetition).

jelbourn · 2017-06-06T00:27:14Z

src/lib/core/bidi/directionality.spec.ts

+describe('Dir directive', () => {
+  beforeEach(async(() => {
+    TestBed.configureTestingModule({
+      declarations: [Dir, TestAppWithDir, DirTest]


FYI you shouldn't put directives you're testing directly into the declarations of the testing module; better to import the NgModule they live in for real (so BidiModule here)

jelbourn · 2017-06-06T00:34:14Z

src/lib/core/bidi/directionality.spec.ts

+  it('should provide itself as Directionality', () => {
+    let fixture = TestBed.createComponent(TestAppWithDir);
+    let testComponent =
+      getDebugNode(fixture.nativeElement.querySelector('dir-test')).componentInstance;


Can use By.directive instead:

const injectedDirectionality = fixture.debugElement.query(By.directive(InjectsDirectionality)).dir;

(here and elsewhere)

jelbourn · 2017-06-06T00:45:28Z

src/lib/autocomplete/autocomplete.spec.ts

          return {getContainerElement: () => overlayContainerElement};
        }},
-        {provide: Dir, useFactory: () => ({value: dir})},
+        {provide: Directionality, useFactory: () => ({value: dir})},


Rather than changing all of these tests to add the Directionality, we now have a MdCommonModule that all of the components import. You can add BidiModule to the imports of the common module.

jelbourn

LGTM

jelbourn · 2017-06-08T16:14:05Z

@EladBezalel just needs rebase

jelbourn · 2017-06-18T02:07:22Z

I've rebased it

jelbourn · 2017-06-18T03:39:15Z

Hmm, changing the actual dir in the tests is causing other tests to fail on Safari.

- Looks at the `html` and `body` elements for `dir` attribute and sets the Directionality service value to it - Whenever someone would try to inject Directionality - if there's a Dir directive up the dom tree it would be provided fixes angular#3600

jelbourn · 2017-06-18T06:10:36Z

Changed Directionality to inject the document and use a fake in the unit tests

jelbourn · 2017-06-19T23:23:01Z

Caretaker note: some people were importing LayoutDirection, so we'll have to update them as part of the sync

angular-automatic-lock-bot · 2019-09-06T14:01:49Z

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 Apr 11, 2017

jelbourn reviewed Apr 13, 2017

View reviewed changes

EladBezalel force-pushed the dir branch 5 times, most recently from 1793720 to 5e6040d Compare April 15, 2017 05:35

EladBezalel closed this Apr 15, 2017

EladBezalel force-pushed the dir branch from 5e6040d to 21f8899 Compare April 15, 2017 05:41

EladBezalel reopened this Apr 15, 2017

EladBezalel force-pushed the dir branch 7 times, most recently from cda2cc9 to ba756f5 Compare April 19, 2017 16:14

jelbourn reviewed Apr 20, 2017

View reviewed changes

jelbourn added pr: needs cleanup / tests labels May 1, 2017

EladBezalel force-pushed the dir branch 5 times, most recently from 1b22426 to d4f0d53 Compare June 3, 2017 19:14

jelbourn reviewed Jun 6, 2017

View reviewed changes

jelbourn added pr: needs review and removed pr: needs cleanup / tests labels Jun 6, 2017

EladBezalel force-pushed the dir branch 2 times, most recently from 742f79f to d712b9d Compare June 6, 2017 17:59

jelbourn approved these changes Jun 8, 2017

View reviewed changes

jelbourn added pr: lgtm and removed pr: needs review labels Jun 8, 2017

EladBezalel mentioned this pull request Jun 9, 2017

RTL documentation missing #4387

Closed

EladBezalel force-pushed the dir branch 2 times, most recently from ce40d79 to ac874fe Compare June 13, 2017 17:13

jelbourn force-pushed the dir branch from ac874fe to 780b976 Compare June 18, 2017 02:05

jelbourn added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Jun 18, 2017

jelbourn force-pushed the dir branch from 780b976 to 5f8c663 Compare June 18, 2017 06:09

jelbourn added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jun 19, 2017

jelbourn merged commit 61d979e into angular:master Jun 19, 2017

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

feat(directionality): a provider to get the overall directionality #4044

feat(directionality): a provider to get the overall directionality #4044

Uh oh!

Conversation

EladBezalel commented Apr 11, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Apr 20, 2017

Uh oh!

EladBezalel commented Apr 20, 2017

Uh oh!

jelbourn left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn Apr 20, 2017 • edited by EladBezalel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn Apr 21, 2017 • 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!

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!

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!

jelbourn commented Jun 8, 2017

Uh oh!

jelbourn commented Jun 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jelbourn commented Jun 18, 2017

Uh oh!

jelbourn commented Jun 18, 2017

Uh oh!

jelbourn commented Jun 19, 2017

Uh oh!

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

Uh oh!

jelbourn left a comment •

edited

Loading

jelbourn Apr 20, 2017 •

edited by EladBezalel

Loading

jelbourn Apr 21, 2017 •

edited

Loading

jelbourn commented Jun 18, 2017 •

edited

Loading