Top left corner triangle for Arabic #241
Conversation
Thank you for the pull request!The Scribe team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :) If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Android rooms once you're in. Also consider joining our bi-weekly Saturday dev syncs. It'd be great to have you! Maintainer checklist |
|
@Linfye thanks for PR, could you please add screenshot from app before and after the fix. |
Done. :) |
| private fun isArabicLanguage(): Boolean { | ||
| val cornerTriangleImageView: ImageView = binding.cornerTriangle | ||
| val cornerCogImageView: ImageView = binding.cornerCog | ||
| val paramsTriangleLayout = binding.cornerTriangle.layoutParams as FrameLayout.LayoutParams | ||
| val paramsCogLayout = binding.cornerCog.layoutParams as FrameLayout.LayoutParams | ||
| val currentLocale = | ||
| requireActivity() | ||
| .resources.configuration.locales | ||
| .get(0) | ||
| val languageCode = | ||
| currentLocale | ||
| .language | ||
| if (languageCode == "ar") { | ||
| paramsTriangleLayout.gravity = Gravity.TOP or Gravity.START | ||
| paramsCogLayout.gravity = Gravity.TOP or Gravity.START | ||
| cornerTriangleImageView.scaleX = -1f | ||
| cornerCogImageView.scaleX = -1f | ||
| Log.d("Debug", "Arabic") | ||
| } else { | ||
| Log.d("Debug", "Not Arabic") | ||
| } | ||
| return languageCode == "ar" | ||
| } |
There was a problem hiding this comment.
we need to make a single responsibility to this method, i.e. making it do single task.
from it's name, 'isArabicLanguage' , which mean it check the language so it should not have logic related to change the UI configuration such as scale/gravity.
so I think we would split it into two method, one to check language and other to change the view data.
checking language would be in separate util function and would be unit tested.
There was a problem hiding this comment.
isArabicLanguage should also be isRTLLanguage so that we can have it be general from the start :)
| private fun isArabicLanguage(): Boolean { | ||
| val cornerTriangleImageView: ImageView = binding.cornerTriangle |
There was a problem hiding this comment.
what about checking layout direction, so we would cover all RTL languages without need to explicitly check each language.
There was a problem hiding this comment.
If this is possible, then yes, but we can also just save a local variable for RTL languages that we support as as of now the plan is just Arabic and eventually Hebrew.
| currentLocale | ||
| .language | ||
| if (languageCode == "ar") { | ||
| paramsTriangleLayout.gravity = Gravity.TOP or Gravity.START |
There was a problem hiding this comment.
for gravity, I think we do not need to handle it, as it already handled in xml, it set to top|end so in RTL it will be automatically be on left of screen.
|
@MahmoudMabrok Could you review it again? I think this is what you mentioned before. 😊 |
|
for me all is fine, just for scalability , if we can use if not stable solution, so PR is fine for me. |
|
This makes sense to me as well, as we actually have |
Is this OK now? 😊 |
Thanks so much for your efforts. You did a great work. For me, it is ready now. |
There was a problem hiding this comment.
Awesome, thanks @Linfye and @MahmoudMabrok! Let's give this another check post localizations and bringing in the Arabic texts 🌐
Contributor checklist
./gradlew lintKotlin detekt testcommand as directed in the testing section of the contributing guideDescription
When the language is set to Arabic, the "installation corner" on the main page will move to the top-left corner. Since internationalization (i18n) has not yet introduced Arabic, you can use "de" to test the code. 😊
This is tested in German since Arabic is not introduced currently.
Related issue