fix: apply correct RTL margins #527

atscott · 2017-12-07T19:34:07Z

Fixes #274

CaerusKaru · 2017-12-12T08:28:06Z

src/lib/api/directionality.ts

+  /** The current 'ltr' or 'rtl' value. */
+  readonly value: Direction = 'ltr';
+
+  constructor(@Optional() @Inject(DIR_DOCUMENT) _document?: any) {


Is there a reason you're not importing the DOCUMENT token from angular/common? This is why Travis is failing on prerender, it doesn't have access to the global browser document object

atscott · 2017-12-12T19:03:24Z

The @angular/cdk was only in dev dependencies (for the demo, I think). I've moved it to the dependency list and am now using the Directionality injectable from the bidi module.

CaerusKaru · 2017-12-12T19:07:59Z

Excellent, I thought it was in the deps because it's available in the rollup globals, but I guess it was left out (this just means you need to do less work to include the cdk).

atscott · 2017-12-13T01:01:07Z

FYI - current broken test is also failing in master

CaerusKaru · 2017-12-14T14:50:02Z

Rebase with master should fix broken test

atscott · 2017-12-14T16:51:49Z

Done, thanks.

atscott · 2018-01-02T21:58:33Z

Friendly post-holiday ping. Anything that needs to be done to get this merged? RTL support is pretty important to my team.

CaerusKaru · 2018-01-02T22:18:18Z

It would be great if you could add a demo that displays RTL in the demo-app, just something simple that documents the capability.

Also, please change your commit message to something in line with the Angular contribution guidelines, found here

Finally, the PR lists this as a partial fix for #274, are there cases that this does not cover, and can you list that in the commit message?

Other than that the PR LGTM, I'll see what I can do to get it merged in the next week or so (it has to go through internal presubmit tests first)

atscott · 2018-01-03T02:34:59Z

Updated commit message (I think).

The "partial fix" in the description was from when I wasn't using the angular cdk dir directive. This should be a full fix of what was requested in that bug.

I haven't looked much yet at where to put this in the demo app. I wasn't able to get the demo app running.

CaerusKaru · 2018-01-03T04:11:08Z

You need to squash your commits into one with a rebase. If there is an issue with the demo-app, can you post it as a new issue so we can take a look?

Also the scope can be empty, i.e. fix: add RTL support

atscott · 2018-01-03T19:22:16Z

Rebased and filed #547

CaerusKaru · 2018-01-04T01:14:34Z

#547 has been patched in #549. Please incorporate the change (it's one line) and add a demo to the demo-app, leaving out the one-line change in your final commit.

atscott · 2018-01-04T04:46:28Z

Added the demo and updated the implementations to subscribe to the direction changes.

CaerusKaru · 2018-01-04T04:51:07Z

src/lib/api/flexbox/layout-gap.ts

    super(monitor, elRef, renderer);

    if (container) {  // Subscribe to layout direction changes
      this._layoutWatcher = container.layout$.subscribe(this._onLayoutChange.bind(this));
    }
+
+    this._directionWatcher = 
+        this._directionality.change.subscribe(() => this._updateWithValue());


This should be the same as the _layoutWatcher subscription:

this._directionality.change.subscribe(this._updateWithValue.bind(this));

CaerusKaru · 2018-01-04T04:51:29Z

Demo looks great, thanks 👍

CaerusKaru

Hey @atscott, I've discussed this with @ThomasBurleson and here's what we need you to do:

Clean up this PR (rebase/refactor as necessary)
Clone the BiDi code from the CDK since it's low-complexity (and remove the explicit dependency)
We'll get this in for the beta.14 release at the beginning of next month

ThomasBurleson · 2018-02-03T14:05:44Z

@atscott - The reason we want to clone the Directionality class is to avoid introducing an explicit dependency on the CDK at this time.

Later, major portions of the library [ e.g. /src/lib/mediaquery] may be migrated to the CDK and then the @angular/flex-layout dependency on the CDK will be acceptable.

Copy bidi module from angular material cdk so there's not a new dependency.

atscott · 2018-02-05T20:28:18Z

Alright, that sounds good. I made those changes (had trouble rebasing so kind of redid the whole thing) and I think everything's all set.

CaerusKaru

LGTM, thanks!

angular-automatic-lock-bot · 2019-09-05T23:03:16Z

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 label Dec 7, 2017

CaerusKaru reviewed Dec 12, 2017

View reviewed changes

atscott force-pushed the atscott/fix-274 branch 2 times, most recently from 20334be to bfb85cc Compare December 12, 2017 18:58

atscott force-pushed the atscott/fix-274 branch from bfb85cc to 79790c9 Compare December 14, 2017 16:33

atscott changed the title ~~partial fix for #274. Apply correct rtl margins~~ Fix for #274. Apply correct rtl margins Jan 2, 2018

atscott force-pushed the atscott/fix-274 branch 2 times, most recently from c719fea to 925216c Compare January 3, 2018 17:47

CaerusKaru changed the title ~~Fix for #274. Apply correct rtl margins~~ fix: apply correct RTL margins Jan 4, 2018

CaerusKaru added the pr: needs review label Jan 4, 2018

atscott force-pushed the atscott/fix-274 branch from 925216c to 3fdc411 Compare January 4, 2018 04:44

CaerusKaru reviewed Jan 4, 2018

View reviewed changes

atscott force-pushed the atscott/fix-274 branch from 3fdc411 to bb700dc Compare January 4, 2018 04:52

CaerusKaru added pr: lgtm This PR has been approved by the reviewer pr: needs presubmit and removed pr: needs review labels Jan 4, 2018

CaerusKaru self-assigned this Jan 4, 2018

CaerusKaru modified the milestones: v5.0.0-beta.15, v5.0.0-beta.14 Feb 2, 2018

CaerusKaru added pr: needs cleanup/tests and removed pr: on hold pr: lgtm This PR has been approved by the reviewer labels Feb 2, 2018

CaerusKaru suggested changes Feb 2, 2018

View reviewed changes

atscott force-pushed the atscott/fix-274 branch from 8aa085d to 30bf807 Compare February 3, 2018 00:15

googlebot added cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ and removed cla: yes labels Feb 3, 2018

atscott force-pushed the atscott/fix-274 branch 4 times, most recently from 8737be3 to 1433946 Compare February 3, 2018 01:11

atscott force-pushed the atscott/fix-274 branch 2 times, most recently from 453062b to 5727752 Compare February 5, 2018 20:26

fix: apply correct RTL margins

818ce91

Copy bidi module from angular material cdk so there's not a new dependency.

atscott force-pushed the atscott/fix-274 branch from 5727752 to 818ce91 Compare February 5, 2018 20:27

CaerusKaru added cla: yes pr: lgtm This PR has been approved by the reviewer and removed cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ pr: needs cleanup/tests labels Feb 5, 2018

CaerusKaru approved these changes Feb 5, 2018

View reviewed changes

CaerusKaru modified the milestones: v5.0.0-beta.14, v5.0.0-beta.13 Feb 6, 2018

angular deleted a comment from googlebot Feb 9, 2018

ThomasBurleson merged commit 7699957 into angular:master Feb 13, 2018

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

fix: apply correct RTL margins #527

fix: apply correct RTL margins #527

Uh oh!

Conversation

atscott commented Dec 7, 2017 • edited by CaerusKaru Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CaerusKaru Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

atscott commented Dec 12, 2017

Uh oh!

CaerusKaru commented Dec 12, 2017

Uh oh!

atscott commented Dec 13, 2017

Uh oh!

CaerusKaru commented Dec 14, 2017

Uh oh!

atscott commented Dec 14, 2017

Uh oh!

atscott commented Jan 2, 2018

Uh oh!

CaerusKaru commented Jan 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atscott commented Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CaerusKaru commented Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atscott commented Jan 3, 2018

Uh oh!

CaerusKaru commented Jan 4, 2018

Uh oh!

atscott commented Jan 4, 2018

Uh oh!

CaerusKaru Jan 4, 2018

Choose a reason for hiding this comment

Uh oh!

CaerusKaru commented Jan 4, 2018

Uh oh!

CaerusKaru left a comment

Choose a reason for hiding this comment

Uh oh!

ThomasBurleson commented Feb 3, 2018

Uh oh!

atscott commented Feb 5, 2018

Uh oh!

CaerusKaru left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

atscott commented Dec 7, 2017 •

edited by CaerusKaru

Loading

CaerusKaru commented Jan 2, 2018 •

edited

Loading

atscott commented Jan 3, 2018 •

edited

Loading

CaerusKaru commented Jan 3, 2018 •

edited

Loading