Added Optional Support for RightToLeft layout direction

pavankataria / SwiftDataTables

A Swift Data Table package, display grid-like data sets in a nicely formatted table for iOS. Subclassing UICollectionView that allows ordering, and searching with extensible options.

MIT License

448 stars 69 forks source link

Added Optional Support for RightToLeft layout direction #38

Closed mmgrt closed 5 years ago

mmgrt commented 5 years ago

Overriding var flipsHorizontallyInOppositeLayoutDirection, and new option in the DataTableConfiguration model

Pull requests

The easiest way to start contributing is searching open issues by help wanted tag.

Add test coverage to the feature or fix if you can. We are more likely to accept new feature pull requests that have related test coverage. This allows us to keep the library stable as we move forward.
Remember to document the new feature. We do not accept new feature pull requests without its associated documentation.
In case of a new feature please update the example project showing the feature.
Please only one fix or feature per pull request. This will increase the chances your feature will be merged.

pavankataria commented 5 years ago

Hi thanks for taking the time to submit a pull request.

I've checked out this pull request, changed to a language that reads right to left, and nothings changed visually, also the UIApplication.shared.userInterfaceLayoutDirection == .rightToLeft still returns false.

Could you explain what this changes visually and how this solves a possible issue ?

Descriptions and screenshots would be helpful!

mmgrt commented 5 years ago

Sorry for not providing sufficient docs, basically overriding flipsHorizontallyInOppositeLayoutDirection and returning true will flip the horizontal scrolling to be from right to left and if there's a columns fixed, it'll maintain that to be right-to-left as well.

UIApplication.shared.userInterfaceLayoutDirection usually works for me to check if I'm in an RTL context, you can test it by overriding the System Language in the scheme to Arabic for example. UPDATE: And this https://github.com/pavankataria/SwiftDataTables/pull/38#issuecomment-512777803

pavankataria commented 5 years ago

could you make a video showcasing the scrolling difference as the scrolling hasn't changed for me when I did change the language context to arabic which is exactly what I Did.

mmgrt commented 5 years ago

Sure! Drive link

If you want it to work with you, what you need to do as an extra step is to add Arabic/ (Any RTL Language) localization to the sample app and test it with scheme edit. Imgur

pavankataria commented 5 years ago

I'm at a point where I don't have a laptop now changing between jobs and haven't pulled the trigger on buying my very own just yet, this is something I am keen to look at asap.

This is great work and will definitely be merged in once I've tweaked some of the variable naming.

pavankataria commented 5 years ago

For example. does this need a delegate method too? All of the properties outlined in the config file can also be configurable by interfacing with it's counter SwiftDataTableDelegate.

mmgrt commented 5 years ago

Thank you and good luck with your career!

Regarding the delegate methods, I wasn't aware of the counter delegate methods for each config prop tbh, if that's a followed pattern, and you think it's worth putting there, I'll commit it.. let me know please.

pavankataria commented 5 years ago

Aha my career has been fantastic so far, it's just being dealt a couple of bad hands last several months, one start up I was with went insolvent, and the other I recently joined went into administration after 7 years of doing business :o !

Anyway, all good. Have another role starting shortly. That's the beautiful thing about iOS development, I feel like it's a thriving market, it's competitive but there's always someone needing an iOS dev.

Back to your question, yes please do add the delegate method in @mmgrt. Would love to get this merged.

mmgrt commented 5 years ago

I can see you love being an iOS developer by doing this library! You deserve the best.

I added the optional delegate method, config getter method and documentation for better describing the change. If there's anything left, I'll be happy to provide it.

pavankataria commented 5 years ago

Was just double checking this feature again and running it on my side; its awesome to see the columns flip too. Without this pull request, while all the text will be RTL the content structures however - aka the columns - within the scrollview will still be LTR. This pull request ensures the scrollview layout direction is flipped too making it truly RTL. I can see how the folks who read RTL languages can benefit from this.

Nice work once again.

pavankataria commented 5 years ago

This feature is now available from version 0.8.1 so update your pods to the latest version 🎉 🎉 You can check out the release notes and tutorial here: https://github.com/pavankataria/SwiftDataTables/releases/tag/0.8.1