Skip to content
This repository was archived by the owner on Mar 21, 2019. It is now read-only.

fix(FlexboxLayout): issue #6156 and replace auto RTL detected by manual #163

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

xlmnxp
Copy link

@xlmnxp xlmnxp commented Mar 4, 2019

PR Checklist

What is the current behavior?

I replaced auto RTL detected by manual detected

What is the new behavior?

I replaced auto RTL detected by manual detected
new you can change direction FlexboxLayout ( by default is LTR ) by setRtl(boolean rtl)
Fixes #6156.

BREAKING CHANGES:

  • add new variable named mIsRtl by default false
  • add getter and setter for RTL value.
  • replace device RTL detected by mIsRtl

@cla-bot cla-bot bot added the cla: yes label Mar 4, 2019
@xlmnxp xlmnxp changed the title Fix issue #6156 and replace auto RTL detected by manual fix(FlexboxLayout): issue #6156 and replace auto RTL detected by manual Mar 4, 2019
@manoldonev
Copy link
Contributor

@xlmnxp your modifications are in java (Android) code while NativeScript/NativeScript#6156 is related to an iOS issue.

Also, I don't think the proposed change of replacing automatic with manual rtl detection for Android has an added benefit.

@xlmnxp
Copy link
Author

xlmnxp commented Mar 13, 2019

@manoldonev I'm working on a project that adds support for RTL layouts
This Flexbox Layout should disable RTL support to work as compatible with the plugin.

https://github.com/nativescript-rtl/ui

@manoldonev
Copy link
Contributor

Ok but this change disables automatic rtl detection for the built-in NativeScript FlexboxLayout even in scenarios where your rtl plugin is not used. I think you should explore other options to achieve the desired effect e.g. implement your new property / override onLayout(...) in your own class that extends FlexboxLayout (as far as I can see that is the approach you are using for the rest of the layout classes).

@xlmnxp
Copy link
Author

xlmnxp commented Mar 13, 2019

@manoldonev FlexboxLayout is an anomaly for other layouts where it is the only one that detect the right-to-left direction automatically and the problem is that that it is only in the android system.
It is best to support automatic detect of the right-to-left direction in all other layouts or remove support from it only

@manoldonev
Copy link
Contributor

@xlmnxp I agree that best option is to support automatic rtl detection out-of-the-box.

As for your PR -- we are not prepared to introduce such kind of breaking change for the FlexboxLayout. Alternative option could be to introduce a three-way rtlMode property (enum or Boolean) that would keep the current automatic behavior as default but would allow you to set it manually for your plugin scenarios.

@xlmnxp
Copy link
Author

xlmnxp commented Mar 18, 2019

@manoldonev ok I'll do it

@manoldonev
Copy link
Contributor

@xlmnxp btw I was wondering did you consider extending the automatic rtl support within the built-in NativeScript core modules via community PR before deciding to implement your plugin?

if yes -- what kind of problems did you encounter?

@SvetoslavTsenov
Copy link
Contributor

SvetoslavTsenov commented Mar 21, 2019

Hey @xlmnxp,
I want to notify you that the NativeScript/tns-core-modules-widgets repository is being moved to the NativeScript/NativeScript repository in an effort to streamline the maintenance and the QA processes. The tns-core-modules-widgets repository will be archived soon. We advise you to open a new PR in the NativeScript/NativeScript with the suggested changes and we will be happy to review it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants