search

oppia / oppia-android

A free, online & offline learning platform to make quality education accessible for all.
https://www.oppia.org
Apache License 2.0
315 stars 517 forks source link

[BUG]: Terms of Service and Privacy Policy content should still be left-aligned, even if the app is in an RTL language. #5039

Open seanlip opened 1 year ago

seanlip commented 1 year ago

Describe the bug

In RTL languages, we are currently keeping our Terms of Service and Privacy policy content in English (so that we can have a canonical version). However, the English text should be left-aligned.

(Thanks to @KolliAnitha for the repro and images, which I took from https://github.com/oppia/oppia-android/issues/5028.)

To Reproduce Steps to reproduce the behavior:

  1. Go to 'Home' - Options
  2. Select App language as Arabic
  3. Go back and click on Help - Terms of Service/ Privacy policy
  4. See that the content is right-aligned despite being in English.

Expected behavior The English content should be left-aligned (but not the note at the bottom of the page linking to the up-to-date policy).

Demonstration Screenshot_20230608-080139

Environment Device/emulator being used: Infinix SMART 5 Android or SDK version (e.g. Android 5 or SDK 21): Android 11 App version (you can get this through system app settings or via the admin controls menu in-app): 0.11-beta-8c81c98d8b

ShubhadeepKarmakar commented 1 year ago

Same thing is happening on the onboardimg screen.

https://github.com/oppia/oppia-android/assets/99060332/f930ef9c-95d9-4142-9680-44486ecdf254

ShubhadeepKarmakar commented 1 year ago

I would like to work on this issue. May I get assigned to it?

adhiamboperes commented 1 year ago

I would like to work on this issue. May I get assigned to it?

What's your proposal for fixing this?

ShubhadeepKarmakar commented 1 year ago

https://github.com/oppia/oppia-android/assets/99060332/3fe3f8dc-9417-4edf-956b-9db8c802bd35

I would like to work on this issue. May I get assigned to it?

What's your proposal for fixing this?

PoliciesFragmentPresenter

These changes are made in the PoliciesFragmentPresenter.kt class and work perfectly but this is not the correct approach I think. DisplayLocaleImpl.kt class was final in nature but I had to make the DisplayLocaleImpl class open in nature to change the PoliciesFragmentPresenter class.

adhiamboperes commented 1 year ago

ltr.mp4

I would like to work on this issue. May I get assigned to it?

What's your proposal for fixing this?

PoliciesFragmentPresenter

These changes are made in the PoliciesFragmentPresenter.kt class and work perfectly but this is not the correct approach I think. DisplayLocaleImpl.kt class was final in nature but I had to make the DisplayLocaleImpl class open in nature to change the PoliciesFragmentPresenter class.

I get the sense that you are trying to force the layout direction to be LTR. You don't need to modify DisplayLocaleImpl or override it's methods to retrieve Layout Direction. AppLanguageResourceHandler has a public api for that, getLayoutDirection(). See StateAssemblerMarginBindingAdapters or SurveyOnboardingBackgroundView for usage examples.

ShubhadeepKarmakar commented 1 year ago

Under the ListItemLeadingMarginSpan class we have the ulSpan class and the olSpan class which takes displayLocal as an input and then retrieves the direction of the layout using the getLayoutDirection() method to get the bullet drawing position. That's why I created an object of the DisplayLocaleImp class and overridden the getLayoutDirection() method to pretend it's LTR. OR We can make another class just for PoliciesTextview which implements Oppia.DisplayLocale class. Everything remains the same as in the DisplayLocaleImp class, we only need to change the getLayoutDirection() method to assert the LTR over the RTL. rtl_page-0001

@adhiamboperes any suggestion?

adhiamboperes commented 1 year ago

Under the ListItemLeadingMarginSpan class we have the ulSpan class and the olSpan class which takes displayLocal as an input and then retrieves the direction of the layout using the getLayoutDirection() method to get the bullet drawing position. That's why I created an object of the DisplayLocaleImp class and overridden the getLayoutDirection() method to pretend it's LTR. OR We can make another class just for PoliciesTextview which implements Oppia.DisplayLocale class. Everything remains the same as in the DisplayLocaleImp class, we only need to change the getLayoutDirection() method to assert the LTR over the RTL. rtl_page-0001

@adhiamboperes any suggestion?

The layout direction retrieved will be the same regardless of the function you call to retrieve it, which is why you should use the public function from AppLanguageResourceHandler and don't reimplement DisplayLocale.

neeldoshii commented 9 months ago

is this issue fixed? @ShubhadeepKarmakar @BenHenning

adhiamboperes commented 9 months ago

@neeldoshii, feel free to suggest a fix for this. Please also refer to https://github.com/oppia/oppia-android/pull/5181#issuecomment-1776935108 for a potential solution.

neeldoshii commented 9 months ago

Sure @adhiamboperes, I will release a fix in my PR based on the comment. Also, is there any communication channel which is used to communicate with other contributors?

adhiamboperes commented 9 months ago

Sure @adhiamboperes, I will release a fix in my PR based on the comment. Also, is there any communication channel which is used to communicate with other contributors?

At the moment, Our discussions page is the best place to communicate.

neeldoshii commented 9 months ago

@Vishwajith-Shettigar I am still working on this. If you don't mind can I work on it?

neeldoshii commented 9 months ago

Hi @adhiamboperes, currently on the debug release we are not able to change the app language?

Is it some kind of issue?

Vishwajith-Shettigar commented 9 months ago

Hi @neeldoshii if you are working on any issue you should be assigned first, please take care of it, I'm closing my PR and assigning you.

Vishwajith-Shettigar commented 9 months ago

Hi @adhiamboperes, currently on the debug release we are not able to change the app language?

Is it some kind of issue?

You are using gradle to build i guess, if you want change app language you have to build app on bazel and for this issue you can change your system language to arabic or something like that.

adhiamboperes commented 9 months ago

Hi @adhiamboperes, currently on the debug release we are not able to change the app language? Is it some kind of issue?

You are using gradle to build i guess, if you want change app language you have to build app on bazel and for this issue you can change your system language to arabic or something like that.

@neeldoshii, as @Vishwajith-Shettigar said, you can instead change your system language to RTL like Arabic which will also change the app language, if you're unable to build bazel.

neeldoshii commented 9 months ago

image

I have worked and able to make the text to have in LTR.

I get the sense that you are trying to force the layout direction to be LTR. You don't need to modify DisplayLocaleImpl or override it's methods to retrieve Layout Direction. AppLanguageResourceHandler has a public api for that, getLayoutDirection(). See StateAssemblerMarginBindingAdapters or SurveyOnboardingBackgroundView for usage examples.

I have followed the instructions based on the mentioned.

I am little confused. If you look closely html <li> tags are having issues. The bullet point are always forcefully there in RTL. If I perform any change in LiTagHandler class and make them forcefully be in left side then it will get broked on some other screen if we use li tags.

Any idea how to solve this?

@adhiamboperes @Vishwajith-Shettigar Thank You

Vishwajith-Shettigar commented 9 months ago

@neeldoshii I doubt that texts are still in proper LTR mode according to the screenshot ( look at last sentence of all paragraphs). Better you create a draft PR.

For li tag, you can overload the method related to li tag for privacy page or your can create a new object of displaylocaleimp and override getLayoutDirection to return LTR. Refer both closed PR.