fix(transition-ios): RTL fix for transition on ios

[sijav]

Short description of what this resolves:

This will fix the transition on ios platform when it's RTL

Changes proposed in this pull request:

• Changed the translateX animations with opposite value if it's on rtl

Ionic Version: 2.x / 3.x

Fixes: #11211

[AmitMY]

Works very well!
I am not very sure about the naming though, as OFF_LEFT_RTL seems to really mean the offset to the right.

Do you think using start and end makes sense for this? like OFF_START, OFF_END, OFF_START_RTL, OFF_END_RTL? (left is start, right is end)

[sijav]

@AmitMY nevermind what I was saying earlier :D it was the worst idea, besides it really is right offset not start offset, but we do it -99.5% instead of 99.5%

[AmitMY]

@sijav Yeah I saw.. I was just about to comment the -- case, but you reverted :)
Keeping it right & left seems reasonable (as two separate cases).
There is a solution to do with only one set of variables, but I am not sure the overhead is worth it.
(have a utility function that gets a number/string, casts it into a string, then checks if the first character is -, and if so removes it, otherwise adds it)

[sijav]

@AmitMY yeah ... it was a very bad solution :D
I don't think it worth it but let me know if anything could be done...

[brandyscarney]

Thanks for the PR! This looks good to me. I want to get one more team member to test it and if it looks good to them we'll merge it. 😄

[AmitMY]

Khalid had a good point. Instead of duplicating the variables, you can do:
OFF_LEFT = this.plt.isRTL ? "-33%" : "33%"

So you only check once (per call) and it is cleaner.

What do u think?

[sijav]

@AmitMY Khalid? I don't see any comment from him, but nevertheless I have nothing against it however I don't know how RTL change can work in runtime but I hope this change won't affect it.

[AmitMY]

@sijav Well, of course that part of it will be to move the constants to the init function (which is also required for the this.plt.isRTL), which I think will mean that change will work (if it works for current code)
Your code works, and is good, it is just those double variables that are tiny bit annoying :) brandy just looks for more reviews to merge it, hopefully soon.

Khalid commented but removed his comment.

[Khalid-Nowaf]

Sorry guys, I just removed my comment after seen my suggestion is already suggested by @AmitMY.

[sijav]

@AmitMY How can I do that?
The OFF_LEFT is a const variable which means we need to declare it's value when we declare the variable and we cannot change it afterward (eg. on init of the class), and where we declare it is on the root of the ts not on any class, and it will be called at the init of the program (when the program is about to run the js), so there's no this.plt available.
EDIT: the only way I can do this is with this code const OFF_LEFT = document.getElementsByTagName('html')[0].getAttribute('dir') == 'rtl' ? "-33%" : "33%" which frankly I can't see any reason why I should do this :|

[AmitMY]

@sijav Haha no not like that :P
Put the constants inside the init method instead of outside, so it will have the same behavior, and there will not be duplication.
+
It will work if you change direction in run time, iff the current fix works in runtime

[AmitMY]

Looks much better! @brandyscarney any other note or can this get merged?

[brandyscarney]

Nice! I have someone else testing it today and if all goes well with their tests I will merge it. 🙂

[mhartington]

Only thing I can see is that the swipe to go back gesture isn't moving the active page, but that issue existed before this PR, so I think it's ok for now.

[sijav]

@mhartington I've made about a dozen PR #11822 will fix that

[brandyscarney]

Perfect, thanks!