feat: rtl support #343

yggg · 2018-04-03T07:42:22Z

Please read and mark the following check list before creating a pull request:

I read and followed the CONTRIBUTING.md guide.
I read and followed the New Feature Checklist guide.

Short description of what this resolves:

nnixaa · 2018-04-04T13:02:59Z

src/framework/theme/components/sidebar/sidebar.component.ts

@@ -47,7 +47,7 @@ export class NbSidebarFooterComponent {
 /**
 * Layout sidebar component.
 *
- * Sidebar can be place on the left or the right side of the layout, can be fixed (shown above the content)
+ * Sidebar can be place on the start or the end side of the layout, can be fixed (shown above the content)


start/end side of the layout - this sounds strange...

nnixaa · 2018-04-04T13:03:06Z

src/framework/theme/components/sidebar/sidebar.component.ts

@@ -63,10 +63,10 @@ export class NbSidebarFooterComponent {
 * </nb-sidebar>
 * ```
 *
- * @example Example of fixed sidebar located on the left side, initially collapsed.
+ * @example Example of fixed sidebar located on the start side, initially collapsed.


start/end side of the layout - this sounds strange...

nnixaa · 2018-04-04T13:06:12Z

src/framework/theme/services/direction.service.ts

+import { InjectionToken, Optional, Inject, Injectable } from '@angular/core';
+import { NbDocument } from '../theme.options';
+
+export type Direction = 'ltr' | 'rtl';


Let's call it as NbDirection

nnixaa · 2018-04-04T13:06:29Z

src/framework/theme/services/direction.service.ts

+
+export type Direction = 'ltr' | 'rtl';
+
+export const LAYOUT_DIRECTION = new InjectionToken<Direction>('Flow direction');


NB_LAYOUT_DIRECTION

nnixaa · 2018-04-04T13:07:32Z

src/framework/theme/theme.module.ts

@@ -55,7 +56,8 @@ export class NbThemeModule {
   */
  static forRoot(nbThemeOptions: NbThemeOptions,
                 nbJSThemes?: NbJSThemeOptions[],
-                 nbMediaBreakpoints?: NbMediaBreakpoint[]): ModuleWithProviders {
+                 nbMediaBreakpoints?: NbMediaBreakpoint[],
+                 layoutDirection?: Direction): ModuleWithProviders {


should we pass it as part of nbThemeOptions?

nnixaa · 2018-04-05T20:23:40Z

@Tibing please review the popover changes. The changes are intended to RTL support.

Tibing · 2018-04-05T20:52:04Z

src/framework/theme/services/direction.service.ts

+    private document: NbDocument,
+    @Optional() @Inject(NB_LAYOUT_DIRECTION) private dir = NbLayoutDirection.LTR,
+  ) {
+    this.dir = dir;


Do we really have to call this variable dir? I know that it called dir in Document but it completely unclear to me. When I see dir I think about directory 😄

Tibing · 2018-04-05T20:53:04Z

src/framework/theme/services/direction.service.ts

+   * Returns current layout direction.
+   * @returns NbLayoutDirection.
+   * */
+  getDirection(): NbLayoutDirection {


Also, if we call direction like dir, why do we call getter like getDirection instead of getDir?

Tibing · 2018-04-05T20:57:27Z

src/framework/theme/components/popover/popover.directive.ts

+   * Maps logical position to physical according to current layout direction.
+   * */
+  private get physicalPlacement(): NbPopoverPlacement {
+    const { isLtr } = this.layoutDirectionService;


Maybe we can write

const isLtr = this.layoutDirectionService();

?
This syntax is great but it's really not obvious why we destructuring service and I have to check the service implementation to find out what's going on.

Tibing · 2018-04-05T21:10:54Z

src/framework/theme/components/popover/popover.directive.ts

@@ -287,7 +295,7 @@ export class NbPopoverDirective implements OnInit, OnDestroy {
  /*
   * Set container position.
   * */
-  private patchPopoverPosition({ top: top, left: left }) {
+  private patchPopoverPosition({ top, left }) {


Do we really need this changes? 😸

Tibing · 2018-04-05T21:17:07Z

src/framework/theme/components/popover/popover.directive.ts

+  /*
+   * Maps logical position to physical according to current layout direction.
+   * */
+  private get physicalPlacement(): NbPopoverPlacement {


I think it's quite complicated solution for rtl support. Maybe we can add START and END in NbPopoverPlacement instead of creating separate enum. And then extend NbPositioningHelper with calculators which can choose between left and right calculators based on the NbLayoutDirectionService.
I think it'll simply and will require fewer changes.
What do you think about this solution?

Tibing · 2018-04-05T21:20:20Z

src/framework/theme/components/layout/layout.component.ts

@@ -43,15 +43,15 @@ import { NbWindow, NbDocument } from '../../theme.options';
 })
 export class NbLayoutColumnComponent {

-  @HostBinding('class.left') leftValue: boolean;
+  @HostBinding('class.start') startValue: boolean;


Could you please also update comment above this component?

Tibing · 2018-04-05T21:25:13Z

src/framework/theme/components/sidebar/sidebar.component.ts

@@ -121,8 +121,8 @@ export class NbSidebarComponent implements OnInit, OnDestroy {
  private alive = true;

  @HostBinding('class.fixed') fixedValue: boolean = false;
-  @HostBinding('class.right') rightValue: boolean = false;
-  @HostBinding('class.left') leftValue: boolean = true;
+  @HostBinding('class.start') startValue: boolean = true;


What should I do If I want to have the sidebar on the left side all the time? I completely agree this is a rare situation but how will I solve it?

@Inject

It's not possible to use @Inject with type. See angular/angular#15640

BREAKING CHANGE: change physical left, right properties to logical start, end respectively

'unset' value isn't supported by IE11

In rtl layout, popover positioned outside of viewport if 'left' set to 'auto'. In such case, content could shrink, so we'll calculate position based on dimentions, that would differ from those, that we'll have when it rendered inside a viewport. With 'left' equals to '0' popover should always feet viewport and dimentions will not change. Details: https://www.w3.org/TR/CSS2/visuren.html#propdef-left https://www.w3.org/TR/CSS2/visudet.html#abs-non-replaced-width

nnixaa

let's add a checklist item to DEV_DOCS

To be able use rtl mixins in nebular and ngx-admin in .component.scss and .theme.scss. We cann't nest multiple host selectors due to big amount of bugs.

…into feature/rtl-support

nnixaa

let's add a test to ensure that dir=ltr is set

…into feature/rtl-support

nnixaa · 2018-05-07T14:57:48Z

docs/articles/rtl.md

@@ -0,0 +1,53 @@
+All Nebular components were updated to support RTL out of the box.


All Nebular components support RTL out of the box.

nnixaa · 2018-05-07T15:02:49Z

docs/articles/rtl.md

@@ -0,0 +1,53 @@
+All Nebular components were updated to support RTL out of the box.
+
+Now every component, that previously could be positioned via some setting, besides old `left` and `right` options, also support logical `start` and `end` properties, similar to flexbox. Value of `start` and `end` depends on current layout direction. For LTR it's left, for RTL - right.


The components that accept a position as a setting now also support logical start and end values, similar to flexbox. Value of start and end depends on current layout direction. For LTR it's left and for RTL - right.
For instance, if we need the sidebar to be positioned logically depending on a language direction, then instead of setting it to left we can set its position to start:

<nb-sidebar start></nb-sidebar>

Document direction could be set through the NbThemeModule.forRoot call. Supported values are 'ltr' and 'rtl'.

nnixaa · 2018-05-07T15:03:53Z

docs/articles/rtl.md

+<div class="note note-info">
+  <div class="note-title">Note</div>
+  <div class="note-body">
+    1. Works only in layout component


Components must be placed inside of the <nb-layoyt></nb-layout> component to correctly support language direction.

nnixaa · 2018-05-07T15:04:27Z

docs/articles/rtl.md

+<div class="note note-info">
+  <div class="note-title">Note</div>
+  <div class="note-body">
+    1. Listen only to changes made through service itself


we probably don't need this note

nnixaa · 2018-05-07T15:06:04Z

docs/articles/rtl.md

+<div class="note note-info">
+  <div class="note-title">Note</div>
+  <div class="note-body">
+    One thing to note, mixins should be called only inside of `:host` mixin or `nb-install-component`.


Please note, the mixins are only available within component :host selector or nb-install-component() mixin if used.

outdated

yggg self-assigned this Apr 3, 2018

yggg requested a review from nnixaa April 3, 2018 07:42

nnixaa reviewed Apr 4, 2018

View reviewed changes

nnixaa added the WIP label Apr 5, 2018

nnixaa requested a review from Tibing April 5, 2018 20:22

Tibing previously requested changes Apr 5, 2018

View reviewed changes

yggg added 23 commits April 9, 2018 12:46

feat(rtl): add rtl properties mixins

f0a461a

feat(rlt): support in actions component

08dcdfa

feat(rtl): support on flip card

9e083db

feat(rtl): add direction service

1f151ea

style(checkbox-component): fix typo

aa0a26e

feat(rtl): checkbox support

18d955f

feat(rtl): add rtl only mixin

f26549b

feat(rlt): add support in context-menu

bc1702f

style(contenxt-menu): change import

e515c32

feat(rtl): badge support

dfba68d

fix(direction-service): remove type from constructor parameter

11f00f9

It's not possible to use @Inject with type. See angular/angular#15640

feat(rtl): add styles for search close button

4a8df51

refactor(badge): add host pseudo class to isolate styles

f1708d8

feat(rtl): add rtl styles for user component

6e94e91

feat(user-component): align text right for rtl layout

d941631

feat(rtl): add sidebar rtl styles

63e0b33

BREAKING CHANGE: change physical left, right properties to logical start, end respectively

feat(rtl): align menu item text to right in rtl layout

e011923

feat(rtl): add mirroring styles for sidebar

d9c05b8

refactor(rtl): use 'auto' to reset position properties

723873a

'unset' value isn't supported by IE11

style: prefix public variables with 'Nb'

14091f5

style: simplify provider entry

c2f6138

style: rename service, use enum instead of type

29a8243

nnixaa reviewed Apr 19, 2018

View reviewed changes

yggg and others added 8 commits April 20, 2018 15:03

refactor(rtl-mixins): simplify rtl mixins

789449a

refactor(theming): use :host instead of :host-context

6bb6b13

To be able use rtl mixins in nebular and ngx-admin in .component.scss and .theme.scss. We cann't nest multiple host selectors due to big amount of bugs.

style: remove unused parameter

521b2f1

refactor: rewrite in one-line selector and add comments

9ee3b24

refactor(direction-service): set dir attribute on document

45e52ab

Merge branch 'feature/rtl-support' of https://github.com/yggg/nebular …

260bd7d

…into feature/rtl-support

test(layout-component): add direction setting test

29a80a0

Merge branch 'master' into feature/rtl-support

b2afe4d

nnixaa requested changes Apr 25, 2018

View reviewed changes

nnixaa and others added 6 commits April 26, 2018 12:06

Merge branch 'master' into feature/rtl-support

7ad6bc9

Merge branch 'master' into feature/rtl-support

f67ac5c

docs(rtl): add bidi support entry in dev docs

b92c056

style: add bidirectionality implemetation longread

386c627

Merge branch 'feature/rtl-support' of https://github.com/yggg/nebular …

b5b29eb

…into feature/rtl-support

docs(rtl): add rtl system overview

c997398

nnixaa requested changes May 7, 2018

View reviewed changes

yggg added 5 commits May 7, 2018 18:30

docs(direction-service): update onDirectionChange description

22699ff

docs(rtl): merge rtl and direction service pages in one

213fb58

docs(rtl): remove temporary notes

2ee07de

docs(rtl): fix direction service docs

01a86dc

docs(rtl): use code snippets syntax

243a7b6

nnixaa previously approved these changes May 8, 2018

View reviewed changes

yggg added 2 commits May 8, 2018 13:28

style: fix typo

c476b78

docs(rtl): remove duplicate service entry

78c0487

yggg dismissed nnixaa’s stale review via 78c0487 May 8, 2018 10:30

nnixaa approved these changes May 8, 2018

View reviewed changes

nnixaa merged commit 0326c1c into akveo:master May 8, 2018

yggg removed the WIP label May 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: rtl support #343

feat: rtl support #343

yggg commented Apr 3, 2018 •

edited

Loading

nnixaa Apr 4, 2018

nnixaa Apr 4, 2018

nnixaa Apr 4, 2018

nnixaa Apr 4, 2018

nnixaa Apr 4, 2018

nnixaa commented Apr 5, 2018

Tibing Apr 5, 2018

Tibing Apr 5, 2018

Tibing Apr 5, 2018

Tibing Apr 5, 2018

Tibing Apr 5, 2018

Tibing Apr 5, 2018

Tibing Apr 5, 2018

nnixaa left a comment

nnixaa left a comment

nnixaa May 7, 2018

nnixaa May 7, 2018

nnixaa May 7, 2018

nnixaa May 7, 2018

nnixaa May 7, 2018


		export type Direction = 'ltr' \| 'rtl';

		export const LAYOUT_DIRECTION = new InjectionToken<Direction>('Flow direction');

		@@ -0,0 +1,53 @@
		All Nebular components were updated to support RTL out of the box.

		@@ -0,0 +1,53 @@
		All Nebular components were updated to support RTL out of the box.

		Now every component, that previously could be positioned via some setting, besides old `left` and `right` options, also support logical `start` and `end` properties, similar to flexbox. Value of `start` and `end` depends on current layout direction. For LTR it's left, for RTL - right.

feat: rtl support #343

feat: rtl support #343

Conversation

yggg commented Apr 3, 2018 • edited Loading

Please read and mark the following check list before creating a pull request:

Short description of what this resolves:

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

nnixaa commented Apr 5, 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

nnixaa left a comment

Choose a reason for hiding this comment

nnixaa left a 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

yggg commented Apr 3, 2018 •

edited

Loading