-
Notifications
You must be signed in to change notification settings - Fork 517
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
Fix #3592: [RTL] High-fi Add support for RTL in audio player and other drawables included in Oppia #3593
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, before merging it, can we add a screenshot of all the screens contacting all the images which are updated here in PR.
Also, should we add test for these?
I m not sure of testcases. And regarding screenshots there are too many files actually to show LTR and RTL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@veena14cs PTAL
@@ -1,6 +1,7 @@ | |||
<vector xmlns:android="https://schemas.android.com/apk/res/android" | |||
android:width="24dp" | |||
android:height="24dp" | |||
android:autoMirrored="true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MohamedMedhat1998 Can you help us out in deciding what should be RTL behaviour of this icon:
- If we do not mirror it it will look exactly same in RTL too.
- If we mirror it fully than the text will be mirrored too which means that the character in foreign language will also get mirrored meaning it won't be a valid character anymore. So definitely we cannot do this.
What are your thoughts? What would be the best solution here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearing my review since the approach overall looks correct, we just need to find a solution for the audio language icon.
Thanks Ben! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@veena14cs I am unblocking you so that this PR can be merged but just undo the change in ic_audio_lang_24px
and after which this PR can be merged.
Thanks @rt4914 reverted changes done in |
Explanation
This PR fixes #3592 . This PR fixes drawables used in Audio Player and also other drawables used in the project.
Screenshot LTR and RTL
Audio Player
.....
....
Profile exit icon in Navigation Drawer
.......
Note : This screenshot only focus on audio player drawbles and not the image in exploration player.
Checklist