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

Bug Fixs & Enhancement of Achievement Screen #5666

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

neeldoshii
Copy link
Contributor

@neeldoshii neeldoshii commented Mar 27, 2024

Description (required)

Related to / Fixes :

Screenshots

No value(no badge) have value(show badge)
image imagel

Commits

e57f666 - This commit just renames the the java file to kotlin so the next commit will have ease in to review using diff for reviewers.
e4977f5 - Migrated to workable kotlin code using Android Studio auto convert and lil manual.
8c1c53f - Removed the ugly nested linear and relative layout to Constraint Layout.
a0a3f23 - Changed the protected to public since unit tests were not able to access it
7e64ca6 - Corrected the String Format to profile page since I changed the level string which got affected on profile page.
5d1ab37 - Badge Implemented & 332f5c5 focused on fix for the previous commit & 7d8a3bf removed the redundant code of xml (do you want me to squash merge this? for ease in rollback )
91c0370 - Changed progress bar color and added the values inside it.

@neeldoshii
Copy link
Contributor Author

image

Hi @nicolas-raoul 👋,

  1. Just to confirm, are we looking to display progress of (images uploaded, images not reverted, images used) as percentages (e.g., 55%) instead of the current format (e.g., 11/20)? If yes, should the percentage be displayed inside the progress bar or is it fine to show it outside?
    Let me know what works best!
    Context : [Bug]: On the personal page, some numerical values are not legible on the smartphone (too small) #5565 (comment)

  2. Is the CI/CD broken?

@nicolas-raoul
Copy link
Member

Images not reverted should be shown as %, but the other metrics should be shown as a fraction.

In the issue my screenshot was misleading but I had left a comment above it:

Screenshot_20241127-073121.png

@nicolas-raoul
Copy link
Member

I believe CI is not broken, for instance #5960 has the green mark. 🙂

@neeldoshii
Copy link
Contributor Author

I believe CI is not broken, for instance #5960 has the green mark. 🙂

Make Sense, can you help me with this test case. I am unable to understand what's leading to failure. Btw PR is ready to review.

@neeldoshii neeldoshii marked this pull request as ready for review November 27, 2024 00:26
@neeldoshii neeldoshii changed the title [WIP] Bug Fixs & Enhancement of Achievement Screen Bug Fixs & Enhancement of Achievement Screen Nov 27, 2024
@nicolas-raoul
Copy link
Member

AchievementsFragmentUnitTests > testShowSnackBarWithRetryFalse FAILED
java.lang.reflect.InvocationTargetException at AchievementsFragmentUnitTests.kt:263

I only had a quick glance but the java/kt look equivalent indeed.

Maybe try to run the unit test in debug mode?

@neeldoshii

This comment was marked as resolved.

@neeldoshii neeldoshii reopened this Dec 1, 2024
@neeldoshii
Copy link
Contributor Author

Test Case Passed finally :-) Ready to merge @nicolas-raoul 😊

The tests were failing because we didn't handled the on case if progress bar binding is null and we try to call the function.

@@ -73,6 +73,7 @@
<item name="drawerHeaderBackground">@color/drawerHeader_background_light</item>
<item name="tutorialBackground">@color/tutorial_background_light</item>
<item name="icon">@color/secondaryTextColor</item>
<item name="colorPrimary">@color/primaryDarkColor</item>
Copy link
Member

Choose a reason for hiding this comment

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

Nor sure if related, but only on this PR several elements that should be bright are show as dark (in bright mode).

Copy link
Member

Choose a reason for hiding this comment

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

Screenshot_20241202-163557~2.png

Copy link
Member

Choose a reason for hiding this comment

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

Also:
Screenshot_20241202-164424.png

Copy link
Member

Choose a reason for hiding this comment

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

Screenshot_20241202-164606.png

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.

2 participants