fix(item): RTL fix for item #11329

sijav · 2017-04-23T11:00:08Z

Short description of what this resolves:

RTL fix (scss changes) for item for wp, ios and android platform

Changes proposed in this pull request:

Add SVG for detail push and fix for RTL (wp,md,ios)

Ionic Version: 2.x / 3.x

Fixes: #11211

RTL fix (scss changes) for list-header for wp, ios and android platform

RTL fix (scss changes) for item including item-detail-push, item, item-devider, icons avatars and thumbnails on items and item-inner fix for wp, ios and android platform

AmitMY · 2017-04-23T13:24:48Z

src/components/item/item.ios.scss

@@ -210,6 +238,14 @@ $item-ios-sliding-content-background:     $list-ios-background-color !default;
    background-position: right ($item-ios-padding-right - 2) center;
    background-size: 14px 14px;
  }
+  [dir="rtl"] .item-ios[detail-push] .item-inner,


Missing line break

AmitMY · 2017-04-23T13:26:22Z

src/components/item/item.md.scss

@@ -135,6 +148,14 @@ $item-md-sliding-content-background: $list-md-background-color !default;
    background-position: right ($item-md-padding-right - 2) center;
    background-size: 14px 14px;
  }
+  [dir="rtl"] .item-md[detail-push] .item-inner,


Missing line break

AmitMY · 2017-04-23T13:28:22Z

src/components/item/item.wp.scss

@@ -140,6 +148,14 @@ $item-wp-sliding-content-background: $list-wp-background-color !default;
    background-position: right ($item-wp-padding-right - 2) center;
    background-size: 14px 14px;
  }
+  [dir="rtl"] .item-wp[detail-push] .item-inner,


Missing line break

Add missing line breaks

Add missing padding left on RTL

AmitMY · 2017-04-23T21:45:57Z

src/components/item/item.md.scss

+  [dir="rtl"] a.item-md:not([detail-none]) .item-inner {
+    @include svg-background-image($item-md-detail-push-svg-rtl);
+    padding-right: 0;
+    padding-left: 32px;


Can you store 32px in a variable?

Everything else looks great to me

@AmitMY I can but every other variable has been stored in theme, should I add a variable in theme and modify themes too? or else where?

I think in the theme file you need to have a variable:
'$x = 32px'
And then it should be in every file (iOS, md, wp) at the top, near all of the other variables, like:
'$x-ios = $x'

*And the name should be 'item-detail-push-padding-end'
Per platform - 'item-{plt}-detail-push-padding-end'

Well ... for now I've just added a variable at the top of the file like padding for divider, I've also name it right instead of end as everything else is right but I agree with you right should be end and left should be start, but this thing should change globally, am I right?

You are end :)
Yes, should change all in a batch replace, but can only do that for version 4, otherwise need to support the old variables as well which is annoying.

Add a new variable and put 32px padding for the detail arrow

This reverts commit 01ff51f.

Add a new variable and put 32px padding for the detail arrow

AmitMY · 2017-04-23T22:22:42Z

@manucorporat Anything to add? this looks done

manucorporat

This looks like a big change in the css, needs review from @brandyscarney

manucorporat · 2017-04-23T23:09:19Z

src/components/item/item.ios.scss

@@ -33,6 +33,12 @@ $item-ios-detail-push-color:              $list-ios-border-color !default;
 /// @prop - Icon for the detail arrow
 $item-ios-detail-push-svg:                "<svg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 12 20'><path d='M2,20l-2-2l8-8L0,2l2-2l10,10L2,20z' fill='#{$item-ios-detail-push-color}'/></svg>" !default;

+/// @prop - Icon for the detail arrow RTL
+$item-ios-detail-push-svg-rtl:            "<svg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 12 20'><path d='M10,20l2-2l-8-8L8-8l-2-2l-10,10L10,10z' fill='#{$item-ios-detail-push-color}'/></svg>" !default;


wonder if we can use transform here?

Yes we can, but it will be hackish ... using svg is much cleaner, I'm making another commit with transform, let me know

manucorporat · 2017-04-23T23:09:39Z

src/components/item/item.ios.scss

@@ -61,6 +67,11 @@ $item-ios-sliding-content-background:     $list-ios-background-color !default;
  transition: background-color 200ms linear;
 }

+[dir="rtl"] .item-ios {
+  padding-left: 0;
+  padding-right: ($item-ios-padding-left);


remove parentesis

remove unnecessary parentesis

Use transform instead of independent svg for item-detail on RTL

AmitMY · 2017-04-24T05:24:12Z

Tiny note:
Merge this PR over those: #11247 and #11246 who are on the same subject, but less covering.

…rtl-fix-item

This reverts commit 3269eac.

This reverts commit 39de5c8.

…t' into rtl-fix-item" This reverts commit 6ee3451, reversing changes made to 3269eac.

This reverts commit 3269eac.

…fix-item

In favor of ionic-team#11342

…fix-item

sijav · 2017-05-13T17:22:53Z

This PR is ready for another review

AmitMY · 2017-05-13T17:39:35Z

@sijav It feels like something was reverted here.. wasn't there a rotate instead duplicate arrows?

sijav · 2017-05-13T18:05:05Z

@AmitMY rotating is very hackish and does not look good, however I could manage to make it work with changing the border and shadow but still it doesn't look good. So I'm just reverse the icon's svg and it's working great, we can also use transform to rotate the icon itself, so let me know which one is preferred.
Also please check that I'm doing the include rtl right thanks.

AmitMY · 2017-05-13T18:06:46Z

@sijav You used it correctly. I just ran e2e.watch and it looks good!
About transforming/using 2 svgs.. @manucorporat what do you think?

sijav · 2017-05-28T08:13:38Z

Any updates?
Anything should I do here?

…l-fix-item

AmitMY · 2017-06-06T17:30:44Z

@sijav This was not merged because it was waiting on another PR.

Now that it is merged, what you need to do is, like you have rtl stuff under @include rtl(), have the ltr stuff under @include ltr() (the svg background image, and the background-position)

sijav · 2017-06-06T19:19:56Z

@AmitMY thanks, I'll do it tomorrow I don't have access to my laptop now, also can you please check the other PRs of mine and tell me if there's any changes I should make to them as well because of ltr mixin? Thanks again 😊

AmitMY · 2017-06-06T19:22:02Z

@sijav Most of your other PRs also use transform which is another mixin, so they can't be changed until that is merged (soon, very soon)

sijav · 2017-06-06T19:31:06Z

@AmitMY I meant these #11865 #11842 #11830 #11825

AmitMY · 2017-06-06T21:12:06Z

Actually, I decided to make the background and position mixins of their own, so this will be generalised
#11945 and #11946

If those will get rejected, this will be approved

sijav · 2017-06-07T02:44:20Z

@AmitMY nice, that's what I expected 👌
Thank you.

brandyscarney · 2017-06-09T20:34:49Z

Thank you for the PR! Closing this as it was fixed by #11945. 😄

sijav added 2 commits April 23, 2017 15:17

fix(list-header): RTL fix for list-header

c932c59

RTL fix (scss changes) for list-header for wp, ios and android platform

fix(item): RTL fix for item

ef53e35

RTL fix (scss changes) for item including item-detail-push, item, item-devider, icons avatars and thumbnails on items and item-inner fix for wp, ios and android platform

AmitMY reviewed Apr 23, 2017

View reviewed changes

sijav added 2 commits April 24, 2017 01:53

refactor(item): add missing line breaks

7099786

Add missing line breaks

fix(item-devider): add missing padding left on RTL

9bcc3be

Add missing padding left on RTL

AmitMY reviewed Apr 23, 2017

View reviewed changes

sijav added 3 commits April 24, 2017 02:40

refactor(item): put 32px padding to a new variable

01ff51f

Add a new variable and put 32px padding for the detail arrow

Revert "refactor(item): put 32px padding to a new variable"

bef8c53

This reverts commit 01ff51f.

refactor(item): put 32px padding to a new variable

55d9213

Add a new variable and put 32px padding for the detail arrow

manucorporat suggested changes Apr 23, 2017

View reviewed changes

manucorporat added the rtl label Apr 24, 2017

sijav added 3 commits April 24, 2017 05:47

refactor(item): remove unnecessary parentesis

61a2330

remove unnecessary parentesis

refactor(item): remove unnecessary parentesis

3b3b698

remove unnecessary parentesis

refactor(item): use transform instead of svg

3269eac

Use transform instead of independent svg for item-detail on RTL

sijav added 7 commits April 25, 2017 09:19

Merge remote-tracking branch 'refs/remotes/origin/rtl-fix-list' into …

6ee3451

…rtl-fix-item

Revert "refactor(item): use transform instead of svg"

39de5c8

This reverts commit 3269eac.

Revert "Revert "refactor(item): use transform instead of svg""

bd513d8

This reverts commit 39de5c8.

Revert "Merge remote-tracking branch 'refs/remotes/origin/rtl-fix-lis…

9144ffb

…t' into rtl-fix-item" This reverts commit 6ee3451, reversing changes made to 3269eac.

Revert "refactor(item): use transform instead of svg"

c2588fd

This reverts commit 3269eac.

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

286d299

…fix-item

wrong svg

096f18e

AmitMY mentioned this pull request May 13, 2017

fix(input): RTL fix for input #11351

Closed

sijav added 3 commits May 13, 2017 21:48

Remove conflicts and other changes than swap the icon

9d6e5a8

In favor of ionic-team#11342

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

4f604f1

…fix-item

fix missing new line at the end of the file

5ff8d3b

Use include rtl instead of another selector

a163f96

brandyscarney added the fix label May 19, 2017

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

c85192d

…l-fix-item

brandyscarney closed this Jun 9, 2017

sijav deleted the rtl-fix-item branch June 16, 2017 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(item): RTL fix for item #11329

fix(item): RTL fix for item #11329

sijav commented Apr 23, 2017 •

edited

Loading

AmitMY Apr 23, 2017

AmitMY Apr 23, 2017

AmitMY Apr 23, 2017

AmitMY Apr 23, 2017

sijav Apr 23, 2017 •

edited

Loading

AmitMY Apr 23, 2017

AmitMY Apr 23, 2017 •

edited

Loading

sijav Apr 23, 2017

AmitMY Apr 23, 2017

AmitMY commented Apr 23, 2017

manucorporat left a comment

manucorporat Apr 23, 2017

sijav Apr 24, 2017 •

edited

Loading

manucorporat Apr 23, 2017

AmitMY commented Apr 24, 2017

sijav commented May 13, 2017

AmitMY commented May 13, 2017

sijav commented May 13, 2017

AmitMY commented May 13, 2017

sijav commented May 28, 2017

AmitMY commented Jun 6, 2017

sijav commented Jun 6, 2017 •

edited

Loading

AmitMY commented Jun 6, 2017

sijav commented Jun 6, 2017

AmitMY commented Jun 6, 2017

sijav commented Jun 7, 2017

brandyscarney commented Jun 9, 2017

fix(item): RTL fix for item #11329

fix(item): RTL fix for item #11329

Conversation

sijav commented Apr 23, 2017 • edited Loading

Short description of what this resolves:

Changes proposed in this pull request:

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

sijav Apr 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmitMY Apr 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmitMY commented Apr 23, 2017

manucorporat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sijav Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmitMY commented Apr 24, 2017

sijav commented May 13, 2017

AmitMY commented May 13, 2017

sijav commented May 13, 2017

AmitMY commented May 13, 2017

sijav commented May 28, 2017

AmitMY commented Jun 6, 2017

sijav commented Jun 6, 2017 • edited Loading

AmitMY commented Jun 6, 2017

sijav commented Jun 6, 2017

AmitMY commented Jun 6, 2017

sijav commented Jun 7, 2017

brandyscarney commented Jun 9, 2017

sijav commented Apr 23, 2017 •

edited

Loading

sijav Apr 23, 2017 •

edited

Loading

AmitMY Apr 23, 2017 •

edited

Loading

sijav Apr 24, 2017 •

edited

Loading

sijav commented Jun 6, 2017 •

edited

Loading