Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jun 19, 2017

Conversation

EladBezalel
Copy link
Member

  • 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 #3600

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 11, 2017
@@ -8,36 +8,42 @@ import {
EventEmitter
} from '@angular/core';

import {Directionality} from './directionality';
export {Directionality} from './directionality';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -8,36 +8,42 @@ import {
EventEmitter
} from '@angular/core';

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

export type LayoutDirection = 'ltr' | 'rtl';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense

})
export class RtlModule {
/** @deprecated */
static forRoot(): ModuleWithProviders {
return {
ngModule: RtlModule,
providers: []
providers: [Directionality]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

*/
@Injectable()
export class Directionality {
value: LayoutDirection;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type def for LayoutDirection should move to this file

import {LayoutDirection} from './dir';

/**
* A provider to get the direction outside of angular on the `html` or `body` elements
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


constructor() {
this.value =
(document.body.getAttribute('dir') ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


constructor() {
this.value =
(document.body.getAttribute('dir') ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jelbourn
Copy link
Member

@EladBezalel ready for another pass?

@EladBezalel
Copy link
Member Author

Yep

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple last things to address

public change = new EventEmitter<void>();

constructor() {
if (document) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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') ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@jelbourn jelbourn Apr 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@jelbourn jelbourn Apr 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

function initDir () {
document.documentElement.dir = '';
document.body.dir = '';
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


TestBed.compileComponents();

initDir();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public change = new EventEmitter<void>();

constructor() {
if (typeof document) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</div>
`
})
class TestAppWithDir {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can just call this ElementWithDir

});
});

describe('Dir directive', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use By.directive instead:

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

(here and elsewhere)

@@ -65,7 +65,7 @@ describe('MdAutocomplete', () => {

return {getContainerElement: () => overlayContainerElement};
}},
{provide: Dir, useFactory: () => ({value: dir})},
{provide: Directionality, useFactory: () => ({value: dir})},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jelbourn
Copy link
Member

jelbourn commented Jun 8, 2017

@EladBezalel just needs rebase

@jelbourn
Copy link
Member

jelbourn commented Jun 18, 2017

I've rebased it

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

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
Copy link
Member

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

@jelbourn 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
Copy link
Member

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

@jelbourn jelbourn merged commit 61d979e into angular:master Jun 19, 2017
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce Directionality injectable
3 participants