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

Comment details approve moderation redesign #20916

Conversation

jarvislin
Copy link
Contributor

@jarvislin jarvislin commented May 30, 2024

See: https://github.com/Automattic/wordpress-mobile/issues/42

Design: yWt5gg3nWORhu079Qfv3mS-fi-1135_5126

This PR implements the redesign of Approved UI. It also refactors some codebase.


To Test:

Approve a comment

  1. Sign in JP app
  2. Go to Notifications tab
  3. Click on a notification with comment type
  4. It should display the approved status UI (if this is an approved comment)
  5. Back to Notifications tab
  6. Use the web console and unapprove the comment you just viewed
  7. Click on the notification again
  8. It should display the pending status UI
  9. Click on the Approve comment button
  10. It should display the approved status UI
  11. Back to Notifications tab
  12. Unapprove the comment and click on the notification again
  13. Click on the More options text
  14. Click on Approved on the bottom sheet.
  15. Go to My site -> More -> Comments -> Comment
  16. Test approved/unapproved comment as above.

Like a comment

  1. Click on an approved comment via Notifications tab
  2. Like/Unlike the comment
  3. It should work as usual.
  4. Go to My site -> More -> Comments
  5. Click on an approved comment
  6. Repeat step 2 & 3.
  7. Done, thank you!

Regression Notes

  1. Potential unintended areas of impact

    • comment, notification
  2. What I did to test those areas of impact (or what existing automated tests I relied on)

    • manual
  3. What automated tests I added (or what prevented me from doing so)

    • unit tests

PR Submission Checklist:

skipped


Testing Checklist (strike-out the not-applying and unnecessary ones):

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • Talkback.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • Large and small screen sizes. (Tablet and smaller phones)
  • Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

@dangermattic
Copy link
Collaborator

dangermattic commented May 30, 2024

2 Warnings
⚠️ strings.xml files should only be updated on release branches, when the translations are downloaded by our automation.
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 30, 2024

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr20916-0b495ec
Commit0b495ec
Direct Downloadjetpack-prototype-build-pr20916-0b495ec.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 30, 2024

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr20916-0b495ec
Commit0b495ec
Direct Downloadwordpress-prototype-build-pr20916-0b495ec.apk
Note: Google Login is not supported on these builds.

@jarvislin jarvislin marked this pull request as ready for review May 30, 2024 15:24
@jarvislin jarvislin assigned antonis and unassigned antonis May 30, 2024
@jarvislin jarvislin requested a review from antonis May 30, 2024 15:24
Copy link

codecov bot commented May 30, 2024

Codecov Report

Attention: Patch coverage is 51.16279% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 41.07%. Comparing base (e3c7f16) to head (0b495ec).

Files Patch % Lines
...android/ui/comments/SharedCommentDetailFragment.kt 0.00% 17 Missing ⚠️
...ress/android/ui/comments/CommentDetailViewModel.kt 84.61% 0 Missing and 4 partials ⚠️
Additional details and impacted files
@@                         Coverage Diff                          @@
##           feature/notifications_refresh_p2   #20916      +/-   ##
====================================================================
+ Coverage                             41.06%   41.07%   +0.01%     
====================================================================
  Files                                  1514     1515       +1     
  Lines                                 69403    69437      +34     
  Branches                              11460    11469       +9     
====================================================================
+ Hits                                  28500    28522      +22     
- Misses                                38324    38332       +8     
- Partials                               2579     2583       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

}
}

object CommentExtension {
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Why did you choose to define the extensions inside an object class?
I'm not suggesting a change here but I usually define extensions on at the package level on a separate file and want to understand the advantage of each approach 🙇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tl;dr

It makes me feel more organized when I wrap them with a object. In fact, the difference is subtle. Both approaches can coexist and be used based on the specific requirements and context of our project.

Approach A (with object)

Pros

  1. Encapsulating related extension functions within a single object helps with modularity and organization.
  2. It is clear which module or functionality these extension functions belong to, enhancing code readability.

Cons

  1. You need to reference CommentExtension when using these extension functions.
  2. Excessive namespaces might increase code complexity and redundancy.

Approach B (without object)

Pros

  1. No extra namespaces, making the code simpler and reducing complexity.
  2. Extension functions can be placed wherever needed without worrying about their enclosing object or class, offering high flexibility.

Cons

  1. If there are many extension functions distributed across different files, it may become difficult to manage and maintain the code.
  2. Extension functions are globally visible, and having the same name in different modules might lead to naming conflicts.

Copy link
Contributor

@antonis antonis left a comment

Choose a reason for hiding this comment

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

Awesome work @jarvislin 🏅
I've tested the implementation on a Pixel 8 Pro (Android 14) in various configurations and everything worked as expected for me. The code also looks great 🎉

Approved Pending Options
Screenshot_20240531_112351 Screenshot_20240531_112444 Screenshot_20240531_112948

Copy link

sonarcloud bot commented May 31, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@jarvislin jarvislin merged commit 4f00ccc into feature/notifications_refresh_p2 May 31, 2024
20 checks passed
@jarvislin jarvislin deleted the issue/comment-details-approve-moderation branch May 31, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants