Skip to content
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

5074: Fix cropped numbers in Leaderboard #5143

Conversation

RitikaPahwa4444
Copy link
Collaborator

Description (required)

Fixes #5074

What changes did you make and why?

Fix without indentation changes:

Enabled auto-scaling of text

The numbers in the Achievements fragment are getting cropped as the app is currently using a hard-coded text size value of 9dp and the text is of the form "xxx/xxx", that is, the circular progress bar can accommodate at most 6 digits and one '/' as of now. Increasing the size of the progress bar might disturb the nearby elements and so, I enabled auto-scaling of the text.

For defining the UI, Commons makes use of the dependency: com.dinuscxj:circleprogressbar:1.1.1 (Available here).

Since this library does not handle larger numbers even in the latest version, they get cropped in the Achievements fragment. Even though we can programatically scale the text size, a more reliable method is to make use of the official Android UI guide for autoscaling text. Modifying the library might introduce an additional maintenance overhead for the app and so, I added a frame layout to make use of the official auto-sizeable TextView. Since the circular progress bar takes up a very small portion of the activity, the GPU considerations are negligible here.

I also changed the unit for progress bar text size fromdp to sp (for text scaling) in the dimens file as this dimension would not impact any other component except the fix.

Tests performed (required)

Tested ProdDebug on Redmi 5A with API level 26.

Screenshots (for UI changes only)

Since I haven't uploaded many pictures on the Commons app, I cannot reproduce the issue directly. However, this is how the XML design shows changes in Android Studio on adding large numbers:

achievements

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much easier now to read the diff, thanks a lot!

@RitikaPahwa4444
Copy link
Collaborator Author

I think the unit tests need to be updated too. Figured out from the failed check on GitHub. Will check and commit if there's any relevant change.

@nicolas-raoul
Copy link
Member

Thanks for noticing the unit tests!

It looks great with 4 digits/4 digits.
Would you mind posting a screenshot showing how it looks with your numbers?

Screenshot_20230213-203107

@RitikaPahwa4444
Copy link
Collaborator Author

RitikaPahwa4444 commented Feb 13, 2023

This is how it looks on my phone:
Achievements_Fragment
[EDIT: Posted the complete screenshot so that the entire scroll view is visible]

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the screenshot looks great!
Looking forward to the unit tests :-)

@nicolas-raoul
Copy link
Member

Did you get a chance to check the unit tests? :-)

fr.free.nrw.commons.profile.achievements.AchievementsFragmentUnitTests > testHideProgressBar FAILED
    java.lang.reflect.InvocationTargetException at AchievementsFragmentUnitTests.kt:260
        Caused by: java.lang.NullPointerException at AchievementsFragmentUnitTests.kt:260

fr.free.nrw.commons.profile.achievements.AchievementsFragmentUnitTests > testSetAchievementsUploadCount FAILED
    java.lang.reflect.InvocationTargetException at AchievementsFragmentUnitTests.kt:273
        Caused by: java.lang.NullPointerException at AchievementsFragmentUnitTests.kt:273
        ```

@RitikaPahwa4444
Copy link
Collaborator Author

I am sorry for the delay; wasn't anticipating any interviews before my continuous exams when I had taken this issue up. Now that my exams have ended today, I will update you soon. Thank you for waiting for me to make the changes :)

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working fine, unit tests passing. Thanks!

@nicolas-raoul nicolas-raoul merged commit 7d895c4 into commons-app:master Feb 26, 2023
@RitikaPahwa4444
Copy link
Collaborator Author

Thank you for reviewing and approving the changes :)

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

Successfully merging this pull request may close these issues.

Numbers cropped in Leaderboard
2 participants