richardtop / CalendarKit

πŸ“… Calendar for Apple platforms in Swift
https://www.youtube.com/watch?v=cJ63-_z1qg8
MIT License
2.49k stars 334 forks source link

RTL support #295

Closed orihpt closed 3 years ago

orihpt commented 3 years ago

About this Pull Request and why it's important

As you may know, in apps that their language is written right-to-left, the layout is mirrored to match the text. In this Pull Request, I added RTL support for the layout, and transitions. That means that the layout will be mirrored when the language is written right-to-left, as it should have been.

Preview

Hebrew (written right-to-left)

drawing

English (written left-to-right)

to show that the layout remains the same in LTR languages

drawing

It's important to remember that this will happen automatically in RTL languages. In all the other languages everything will remain the same.

What did I add RTL support to

I added RTL support to:

If I forgot something please comment and I'll add support for it.

How can the code know if the app language is written RTL

I used the next code to determine that:

let rtl = UIView.userInterfaceLayoutDirection(for: self.semanticContentAttribute) == .rightToLeft

I found it working even if the app language is different than the device language.


I also suggest that next time you should use constraints instead of points on the screen, it would make everything way easier. Thanks for reviewing! πŸ˜ƒ

orihpt commented 3 years ago

161

richardtop commented 3 years ago

Hi, thanks for your help! Overall, looks very promising, I've made just a few "cosmetic" comments, nothing structurally important has been left.

I also suggest that next time you should use constraints instead of points on the screen, it would make everything way easier. Thanks for reviewing! πŸ˜ƒ

It's a good idea, CalendarKit uses frame calculations instead of AutoLayout in some parts mainly for legacy reasons. All of new code is written with AutoLayout if possible.

Looking forward to you adding the changes I've requested and then merging and releasing your change as a part of the new CalendarKit.

orihpt commented 3 years ago

Please check if it's okay now

richardtop commented 3 years ago

Hi, this looks great, thanks for the quick fixes!

I found few other major UI issues that need to be fixed before merging:

  1. Issue with EventView layout (see the comments above)
  2. Incorrect handling of Editing state, the relevant code is in this line: https://github.com/richardtop/CalendarKit/blob/a63f496e08d13b9cb96f0808a35bf8c97f0ed393/Source/Timeline/TimelinePagerView.swift#L221

Please, modify it to support RTL languages as well, if you'd like editing to be included in your pull request. Although, this one is not a deal breaker and your PR is good to go after fixing the first issue.

The style property could also be renamed to leadingInset to reflect the change required to support this feature.

Simulator Screen Shot - iPhone 12 Pro - 2020-12-30 at 02 57 08