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

feat: show the proper message in the custom picker when all images are either uploaded or marked. #6095

Conversation

yuvraj-coder1
Copy link
Contributor

@yuvraj-coder1 yuvraj-coder1 commented Jan 2, 2025

Fixes #5619

Added a message "Congratulations, all pictures in this album have been either uploaded or marked as not for upload" in the custom image picker when the user have either uploaded or marked all the images in an album.

Tests performed

Tested ProdDebug on Vivo2334 with API level 34.

Screenshots (for UI changes only)

WhatsApp.Video.2025-01-02.at.16.50.04_37e0bc04.mp4

…ave been either uploaded or marked as not for upload." in the custom picker when all images are either uploaded or marked.
@Sujal-Gupta-SG
Copy link
Contributor

Sujal-Gupta-SG commented Jan 2, 2025

@nicolas-raoul

Review by GSoC applicant

  • Issue before merging
Record_2025-01-02-21-37-59.mp4
  • Issue after merging
Record_2025-01-02-21-56-17.mp4

Tests performed (required)

Tested on:

  • Build variant: ProdDebug
  • Device: Realme Narzo 20
  • API Level: 30-ext15

Result: The issue was resolved after merging the pull request.

Sujal-Gupta-SG pushed a commit to Sujal-Gupta-SG/apps-android-commons that referenced this pull request Jan 2, 2025
…ave been either uploaded or marked as not for upload." in the custom picker when all images are either uploaded or marked.

check completed pr commons-app#6095
@yuvraj-coder1
Copy link
Contributor Author

Review by GSoC applicant

Hi @yuvraj-coder1 ,

I noticed that you're working on #5619 issue, but it hasn’t been assigned to you yet. To ensure proper collaboration and avoid duplicate efforts, it’s important to first ask for the issue to be assigned to you.

You can comment on the issue with something like: "Hi, can you please assign this issue to me? I’d like to work on it as part of my contributions."

Once the issue is assigned to you, you can proceed with your implementation

hi @Sujal-Gupta-SG ,
I believe there might be a misunderstanding, as the issue (#5619) has already been assigned to me.

@neeldoshii
Copy link
Contributor

neeldoshii commented Jan 3, 2025

Discussion :
Just as an enhancement discussion (we can add this once this is merged). Displaying only text addresses the core problem but can feel a bit stark with the rest of the screen being completely empty. We could explore options like adding a supporting graphic, a hint of color, or even a small interactive element to make it more visually cohesive. WDYT

Maybe it could even suggest the user to perform peer reviews or go take nearby pictures, with buttons to launch these activities.

Also a CTA leading to perform peer reviews would also be best.

@yuvraj-coder1
Copy link
Contributor Author

yuvraj-coder1 commented Jan 3, 2025

Discussion : Just as an enhancement discussion (we can add this once this is merged). Displaying only text addresses the core problem but can feel a bit stark with the rest of the screen being completely empty. We could explore options like adding a supporting graphic, a hint of color, or even a small interactive element to make it more visually cohesive. WDYT

Maybe it could even suggest the user to perform peer reviews or go take nearby pictures, with buttons to launch these activities.

Also a CTA leading to perform peer reviews would also be best.

Thank you for the suggestion, I agree that adding a supporting graphic would enhance the visual appeal and make the screen feel more engaging. I'll explore options for incorporating one above the text.

As for the CTA related to peer reviews, could you clarify a bit further?

Copy link
Contributor

@Sujal-Gupta-SG Sujal-Gupta-SG left a comment

Choose a reason for hiding this comment

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

Review by GSoC applicant

I think the changes is Great

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.

Would you mind checking this unit test?

ImageFragmentTest > testOnCreateView FAILED
    java.lang.IllegalStateException at ImageFragmentTest.kt:124

@nicolas-raoul
Copy link
Member

In a folder which contains both actioned and actionable pictures, when I disable Show already actioned pictures I briefly see the congratulation message before the actionable pictures get shown.

Would you mind showing the congratulation message only once we are sure there are no picture to show?

Thanks! :-)

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 great!
I just left two minor things.

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! :-)

@nicolas-raoul nicolas-raoul merged commit 1c6ebaf into commons-app:main Jan 18, 2025
1 check passed
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.

Add label "Congratulations, all pictures in this album have been either uploaded or marked as not for upload."
4 participants