Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep using default navigation animation direction with RTL languages #520

Merged
merged 7 commits into from Dec 27, 2018

Conversation

@ManueGE
Copy link
Contributor

ManueGE commented Aug 28, 2018

If the user is using an RTL (right to left) language, the navigation controller is expected to use the inverse transition directions by default: .right for pushing and .left for popping.

Currently, it is not working, and the navigation controllers behave as it does in LTR languages.

This commit fixes this issue.

@SD10

This comment has been minimized.

Copy link
Member

SD10 commented Aug 29, 2018

Hey @ManueGE,

Thanks for this PR 👍 I'll have to dig a bit deeper regarding this one. My first thought is -- are you able to easily achieve this effect at the client level?

I don't really like the idea of changing the behavior of .left and .right based on the user's language. I think these directions should be taken quite literally -- as they are with the Apple APIs.

I think it's better to use the terms .leading and .trailing for directions that change based on a users language

@ManueGE

This comment has been minimized.

Copy link
Contributor Author

ManueGE commented Aug 30, 2018

I'm actually not changing the behavior of .left or .right, I am just choosing one of them for the .auto animation based on the user language.

Adding a .leading and .trailing direction maybe is a little more semantic, so we can go this way too.

I can fix it at client level easily, but anyway I am using my own fork of Hero in my app because it has another change I needed (#513), so it is not blocking me at all.

Thanks.

@ManueGE

This comment has been minimized.

Copy link
Contributor Author

ManueGE commented Aug 30, 2018

I added .leading and .trailing, but instead of adding a new case in the enum I made them computed static vars, so they can be used as cases.

Also added .leadingToTrailing and .trailingToLeading in CascadeDirection.

What do you think?

@SD10

This comment has been minimized.

Copy link
Member

SD10 commented Aug 30, 2018

@ManueGE This seems both reasonable and flexible to me, thanks 👍 Going to see if anyone else has some input on this issue so that I'm not dictating a decision

@SD10 SD10 requested a review from lkzhao Aug 30, 2018
@ManueGE

This comment has been minimized.

Copy link
Contributor Author

ManueGE commented Dec 18, 2018

Any new with this PR?

@@ -249,9 +259,9 @@ class DefaultAnimationPreprocessor: BasePreprocessor {
if animators.contains(where: { $0.canAnimate(view: toView, appearing: true) || $0.canAnimate(view: fromView, appearing: false) }) {
defaultAnimation = .none
} else if inNavigationController {
defaultAnimation = presenting ? .push(direction:.left) : .pull(direction:.right)
defaultAnimation = presenting ? .push(direction:.leading) : .pull(direction:.trailing)

This comment has been minimized.

Copy link
@SD10

SD10 Dec 18, 2018

Member

@ManueGE The only thing that concerns me is now we're changing the behavior of this animation. I agree that it's nice to change things based on the userInterfaceLayoutDirection -- but a Hero user could be relying on this not being changed

This comment has been minimized.

Copy link
@ManueGE

ManueGE Dec 18, 2018

Author Contributor

You are totally right, I understand that some users could be relying on this even if maybe it's not the "right" behavior. I saw it as a bug fix, but I understand your point.

This comment has been minimized.

Copy link
@SD10

SD10 Dec 18, 2018

Member

Can we make this configurable so that users can opt-in to the old behavior?

This comment has been minimized.

Copy link
@ManueGE

ManueGE Dec 18, 2018

Author Contributor

Let me think about it a little bit. Will try to figure out something tomorrow

This comment has been minimized.

Copy link
@ManueGE

ManueGE Dec 18, 2018

Author Contributor

I pushed a change. I kept the "left to right" default behavior, but "user interface" and "right to left" behaviors can be set using the defaultTransitionDirectionStrategy property of HeroTransition.

This comment has been minimized.

Copy link
@SD10

SD10 Dec 22, 2018

Member

Thanks @ManueGE. I skimmed this earlier this week. This looks totally reasonable 👍 Will review tomorrow and hopefully merge + release!

} else if inTabBarController {
defaultAnimation = presenting ? .slide(direction:.left) : .slide(direction:.right)
defaultAnimation = presenting ? .push(direction:.leading) : .pull(direction:.trailing)

This comment has been minimized.

Copy link
@SD10

SD10 Dec 18, 2018

Member

Why did we change from .slide to .push here?

This comment has been minimized.

Copy link
@ManueGE

ManueGE Dec 18, 2018

Author Contributor

oops, you are right. My bad, will fix it.

This comment has been minimized.

Copy link
@ManueGE

ManueGE Dec 18, 2018

Author Contributor

Fixed

@SD10

This comment has been minimized.

Copy link
Member

SD10 commented Dec 18, 2018

@ManueGE Yeah, I was able to review this a bit and left you some comments above. Sorry I've been swamped at work for months now 😞 If you need me to look at something the best way is to ping me on GitHub or DM me on Twitter 😓

@ManueGE ManueGE force-pushed the Jaumo:rtl branch from ea114d3 to ed8b969 Dec 18, 2018
@ManueGE ManueGE force-pushed the Jaumo:rtl branch from ed8b969 to 8aeca95 Dec 18, 2018
Manuel G-Estañ and others added 5 commits Dec 18, 2018
right to left and user interface behaviours can be set using the `defaultTransitionDirectionStrategy` property.
@SD10
SD10 approved these changes Dec 27, 2018
Copy link
Member

SD10 left a comment

⛵️

@SD10 SD10 merged commit faf8475 into HeroTransitions:master Dec 27, 2018
JoeMatt added a commit that referenced this pull request Oct 29, 2019
- Fix iOS demo app failing and style on iOS 13
- Fix lint warnings and build errors in demo app
- Add extra metadata to podspec
- Deprecated messages to renamed
- Add Joe Mattiello into Podspec authors for publishing
- Fix pod lib lint failures
- Update Podspec imports to match source imports
- Use more minimal import
- Remove Swift files from framework bundle products
- Remove access modifier warnings (#616)
- GitIgnore xcode log files
- Docs - Run jazzy against new spec
- Docs - Update jazzy config
- Bump version to 1.5.0
- Set theme jekyll-theme-midnight
- Use custom snapshot for views that implements HeroCustomSnapshotView (#541)
- Keep using default navigation animation direction with RTL languages (#520)
- Hidden subviews not taken in account in optimized snapshot type (#521)
- Update Collection 2.0 (#553)

Signed-off-by: Joe Mattiello <git@joemattiello.com>

# gpg: Signature made Mon Oct 28 21:28:30 2019 EDT
# gpg:                using RSA key 8D361039DCE5C34A90E4E466A4679EBF9DE83365
# gpg: Good signature from "Joseph Mattiello <mail@joemattiello.com>" [ultimate]
# gpg:                 aka "Joseph Mattiello <git@joemattiello.com>" [ultimate]
# gpg:                 aka "Joseph Mattiello <joe@provenance-emu.com>" [ultimate]
# gpg:                 aka "Joseph Mattiello <jmattiello@newscorp.com>" [ultimate]
# gpg:                 aka "Joseph Mattiello (Public Git repo commit identity) <git@joemattiello.com>" [ultimate]

# Conflicts:
#	Sources/Extensions/UIView+Hero.swift
#	Sources/Extensions/UIViewController+Hero.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.