fix(menu): rtl fix for menu #11830

sijav · 2017-05-28T15:17:12Z

Short description of what this resolves:

This will fix the menu for rtl direction.

Changes proposed in this pull request:

fix min, max values for RTL (it should be exact opposite than LTR)
fix the left right in scss (when we say right it must be stay as right)

Ionic Version: 3.x

Fixes: #11211

This will complete after: #11822

AmitMY · 2017-05-28T15:41:59Z

Add "// scss-lint:disable PropertySpelling" above 'left: auto' so it will pass linting
(Left and right are not allowed to be used except for special cases like this and like fab, this comment makes this a special case)

AmitMY · 2017-06-06T19:35:53Z

Remove the css changes, as they are conflicting, and this will be reviewed soon

sijav · 2017-06-06T19:40:26Z

@AmitMY what multidir mixin does? It just use the same code for both ltr and rtl? Can I use it in my other PRs?

AmitMY · 2017-06-06T19:45:08Z

multi-dir makes it:

[dir="ltr"] selector, [dir="rtl"] selector {
  // something that is direction sensitive
}

Otherwise, specificity rules can override it.

You can and should use it in your PRs, only on directional properties. I have a list of all PRs and correct order to review them, and what relies on what. Your other PRs show as "not ready" for me as of a small dependency. I will update you as soon as that is taken care of

Left side is without it, middle is with, right is differences: http://ionic-snapshot-go.appspot.com/ionic2/snapshots/d4t/hhp/chrome_400x800

…l-fix-menu

This reverts commit 87c3302.

…l-fix-menu # Conflicts: # src/components/menu/menu.scss

sijav · 2017-06-07T06:58:38Z

@AmitMY waiting for another review, I've tested start and end and it worked great as well.

AmitMY · 2017-06-07T07:20:45Z

This goes well with #11336, which is not done becasue of bad animation after changing direction. Mind having a look? (updatePosition method is what changes the animation, but it breaks it if it runs more than once)

AmitMY · 2017-06-07T09:32:16Z

src/components/menu/menu.scss

+  @include multi-dir() {
+    left: 0;
+    right: auto;
+  }


Leave menu.scss unchanged
The correct default is start, and thus 0, auto, 0, 0 is correct
You are correct that there is a need to address left, so #11953

Sorry for all of the mess around this.

@AmitMY Oh ... alright

This reverts commit f657369.

sijav · 2017-06-07T10:02:44Z

@AmitMY I reverted the commit but if you mind please check the left menu in tablet mode it doesn't open at an appropriate position also about the #11336 commit I'll take a look but there's a lot of things changed which I'm not sure why are these things changed? why we are using a custom animation here? would you mind explaining a bit about your changes?

AmitMY · 2017-06-07T19:14:50Z

@sijav I added an explenation to the body of that PR, as it is more relevant there. About left menu, I think it is fixed with #11953

AmitMY · 2017-06-07T20:53:06Z

src/components/menu/menu-gestures.ts

@@ -57,14 +57,14 @@ export class MenuContentGesture extends SlideEdgeGesture {
  }

  onSlide(slide: SlideData, ev: any) {
-    const z = (this.menu.isRightSide ? slide.min : slide.max);
+    const z = ((this.menu.isRightSide && !this.plt.isRTL) || (!this.menu.isRightSide && this.plt.isRTL) ? slide.min : slide.max);


(this.menu.isRightSide && !this.plt.isRTL) || (!this.menu.isRightSide && this.plt.isRTL)
is logically equal to:
this.menu.isRightSide !== this.plt.isRTL

Can you fix this in all of the expressions in this PR?

@AmitMY well technically yes but I put it that way for more readability ...
Ok I will change all of em

…l-fix-menu

sijav · 2017-06-08T10:48:01Z

I can confirm that menu scss problems has been fixed by #11953 Thanks @AmitMY
About the #11336 I cannot understand why such big changes needed for this? the only thing that does not works with start and end is closing and opening them right? does your commit add any extra features that we don't have right now apart from start and end support?

…l-fix-menu

AmitMY · 2017-06-08T14:58:29Z

src/components/menu/menu-gestures.ts

@@ -89,7 +89,7 @@ export class MenuContentGesture extends SlideEdgeGesture {

  getElementStartPos(slide: SlideData, ev: any) {
    const menu = this.menu;
-    if (menu.isRightSide) {
+    if (this.menu.isRightSide !== this.plt.isRTL) {


Can you please change this.menu to menu?

@AmitMY yeah sure sorry my bad

I missed it too :) orders from above

…x-menu

sijav added 3 commits May 28, 2017 07:58

Merge remote-tracking branch 'refs/remotes/driftyco/master'

39909d0

Merge remote-tracking branch 'refs/remotes/driftyco/master'

e526ce1

fix(menu): rtl fix for menu

27af37b

disable PropertySpelling in scss-lint for this special case

87c3302

brandyscarney added fix labels May 30, 2017

This was referenced Jun 5, 2017

fix(slide-gesture): RTL fix for slide-gesture #11822

Merged

fix(item): RTL fix for item #11329

Closed

sijav added 5 commits June 7, 2017 10:15

Merge remote-tracking branch 'refs/remotes/ionic-team/master' into rt…

db335a1

…l-fix-menu

Revert "disable PropertySpelling in scss-lint for this special case"

851900d

This reverts commit 87c3302.

remove scss changes because of the conflict

cf5534f

Merge remote-tracking branch 'refs/remotes/ionic-team/master' into rt…

47f5093

…l-fix-menu # Conflicts: # src/components/menu/menu.scss

Fix left (default) menu position

f657369

AmitMY reviewed Jun 7, 2017

View reviewed changes

Revert "Fix left (default) menu position"

ddfba2d

This reverts commit f657369.

Merge remote-tracking branch 'refs/remotes/ionic-team/master'

6696bbb

AmitMY reviewed Jun 7, 2017

View reviewed changes

sijav added 2 commits June 8, 2017 14:26

Merge remote-tracking branch 'refs/remotes/ionic-team/master' into rt…

c7dbe53

…l-fix-menu

trim down ifs

a3a0aed

Merge remote-tracking branch 'refs/remotes/ionic-team/master' into rt…

070097a

…l-fix-menu

AmitMY reviewed Jun 8, 2017

View reviewed changes

sijav added 2 commits June 8, 2017 19:31

Merge remote-tracking branch 'refs/remotes/origin/master' into rtl-fi…

3060a1b

…x-menu

remove unnecessary this

88491df

manucorporat merged commit 30047f0 into ionic-team:master Jun 9, 2017

sijav deleted the rtl-fix-menu branch June 16, 2017 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(menu): rtl fix for menu #11830

fix(menu): rtl fix for menu #11830

sijav commented May 28, 2017 •

edited

Loading

AmitMY commented May 28, 2017

AmitMY commented Jun 6, 2017

sijav commented Jun 6, 2017

AmitMY commented Jun 6, 2017

sijav commented Jun 7, 2017

AmitMY commented Jun 7, 2017

AmitMY Jun 7, 2017

sijav Jun 7, 2017 •

edited

Loading

sijav commented Jun 7, 2017 •

edited

Loading

AmitMY commented Jun 7, 2017

AmitMY Jun 7, 2017

sijav Jun 8, 2017 •

edited

Loading

sijav commented Jun 8, 2017 •

edited

Loading

AmitMY Jun 8, 2017

sijav Jun 8, 2017

AmitMY Jun 8, 2017

fix(menu): rtl fix for menu #11830

fix(menu): rtl fix for menu #11830

Conversation

sijav commented May 28, 2017 • edited Loading

Short description of what this resolves:

Changes proposed in this pull request:

AmitMY commented May 28, 2017

AmitMY commented Jun 6, 2017

sijav commented Jun 6, 2017

AmitMY commented Jun 6, 2017

sijav commented Jun 7, 2017

AmitMY commented Jun 7, 2017

AmitMY Jun 7, 2017

Choose a reason for hiding this comment

sijav Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

sijav commented Jun 7, 2017 • edited Loading

AmitMY commented Jun 7, 2017

AmitMY Jun 7, 2017

Choose a reason for hiding this comment

sijav Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

sijav commented Jun 8, 2017 • edited Loading

AmitMY Jun 8, 2017

Choose a reason for hiding this comment

sijav Jun 8, 2017

Choose a reason for hiding this comment

AmitMY Jun 8, 2017

Choose a reason for hiding this comment

sijav commented May 28, 2017 •

edited

Loading

sijav Jun 7, 2017 •

edited

Loading

sijav commented Jun 7, 2017 •

edited

Loading

sijav Jun 8, 2017 •

edited

Loading

sijav commented Jun 8, 2017 •

edited

Loading