The redesign of comment details content #20798

jarvislin · 2024-05-10T14:07:44Z

See https://github.com/orgs/Automattic/projects/944?pane=issue&itemId=60382108
Design: yWt5gg3nWORhu079Qfv3mS-fi-1135_4600
Slack discussion: p1715785205781899/1715757510.947859-slack-C06BWNSR02K

This PR mainly implements the UI redesign of comment details content. I also removed lot of logic related to unused UI components at the moment, some of the logic will come back in modern Kotlin way when I handle the comment moderation tasks.

Screenshot 1	Screenshot 2

To Test:

Sign in JP app
Go to Notifications tab
Click on a notification with Comment type
The UI should be the same as the design on Figma
Click on the icon with three dot ...
It should open the comment menu.
Click on User Info
It should display the user info bottom sheet.
The functionalities on the bottom sheet should be working as before.
Open the comment menu again.
Click on Edit comment.
It should navigate you to the screen of Edit comment.
The functionalities in this screen should be working as before.
Go to My Site -> More -> Comments, click on a comment
Repeat step 5 - 13.
Done, thank you!

P.S. The User Info bottom sheet displays different UI when we are from a comment or a notification because a notification contains richer information in its data.

Regression Notes

Potential unintended areas of impact
- comment, notification
What I did to test those areas of impact (or what existing automated tests I relied on)
- manual
What automated tests I added (or what prevented me from doing so)
- only updated some broken unit tests

PR Submission Checklist:

Skipped

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

…tails-content # Conflicts: # WordPress/src/main/res/layout/comment_detail_fragment.xml

dangermattic · 2024-05-10T14:08:16Z

	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 · 2024-05-10T14:27:18Z

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr20798-1b18034
	Commit	`1b18034`
	Direct Download	`wordpress-prototype-build-pr20798-1b18034.apk`

Note: Google Login is not supported on these builds.

wpmobilebot · 2024-05-10T14:29:39Z

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr20798-1b18034
	Commit	`1b18034`
	Direct Download	`jetpack-prototype-build-pr20798-1b18034.apk`

Note: Google Login is not supported on these builds.

codecov · 2024-05-10T14:39:05Z

Codecov Report

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

Project coverage is 40.73%. Comparing base (d8edd7a) to head (1b18034).
Report is 148 commits behind head on feature/notifications_refresh_p2.

Files	Patch %	Lines
...id/ui/notifications/blocks/CommentUserNoteBlock.kt	0.00%	36 Missing ⚠️
...d/ui/comments/unified/CommentActionPopupHandler.kt	0.00%	20 Missing ⚠️
...s/android/ui/comments/SiteCommentDetailFragment.kt	0.00%	9 Missing ⚠️
...ress/android/ui/engagement/UserProfileViewModel.kt	0.00%	0 Missing and 1 partial ⚠️
...android/ui/notifications/blocks/HeaderNoteBlock.kt	50.00%	1 Missing ⚠️

Additional details and impacted files

@@                         Coverage Diff                          @@
##           feature/notifications_refresh_p2   #20798      +/-   ##
====================================================================
+ Coverage                             40.72%   40.73%   +0.01%     
====================================================================
  Files                                  1490     1492       +2     
  Lines                                 68612    68604       -8     
  Branches                              11364    11348      -16     
====================================================================
+ Hits                                  27942    27946       +4     
+ Misses                                38151    38141      -10     
+ Partials                               2519     2517       -2

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

jarvislin · 2024-05-15T10:56:38Z

WordPress/src/main/java/org/wordpress/android/ui/engagement/UserProfileBottomSheetFragment.kt

+        }
+    }
+
+    private fun UserProfileBottomSheetBinding.setup(state: UserProfileUiState) {


I only moved the code snippets to this function, the logic is not changed.

jarvislin · 2024-05-15T11:01:40Z

WordPress/src/main/java/org/wordpress/android/ui/notifications/blocks/CommentUserNoteBlock.kt

@@ -23,29 +21,29 @@ import org.wordpress.android.util.image.ImageType
 // A user block with slightly different formatting for display in a comment detail
 @Suppress("LongParameterList")
 class CommentUserNoteBlock(


Most of the changes are related to renaming variables and removing some unused functions in this file.

jarvislin · 2024-05-15T11:06:48Z

WordPress/src/main/java/org/wordpress/android/ui/comments/CommentDetailFragment.java

@@ -214,7 +212,6 @@ public View onCreateView(
    ) {
        mBinding = CommentDetailFragmentBinding.inflate(inflater, container, false);
        mReplyBinding = mBinding.layoutCommentBox;
-        mActionBinding = CommentActionFooterBinding.inflate(inflater, null, false);


In this file, I removed a lot of logic related to mActionBinding which is no longer used.

justtwago

Amazing work, Jarvis! 🚀
I tested this on the Google Pixel 7 Pro, and it works perfectly. The only issue I found is that some images are not stretched as they are in the Reader, and custom emojis are sometimes not displayed. This is out of scope since it behaves the same way in trunk, but it’s worth mentioning anyway.

Since the PR is pretty large, I would ask to wait for an extra thorough review from Antonis to make sure it works well and I haven’t missed anything.

WordPress/src/main/java/org/wordpress/android/ui/comments/NotificationCommentDetailFragment.kt

justtwago · 2024-05-18T00:09:34Z

WordPress/src/main/java/org/wordpress/android/ui/comments/NotificationCommentDetailFragment.kt

+
+
+        return BottomSheetUiState.UserProfileUiState(
+            userAvatarUrl = mNote!!.iconURL,


I'm not sure if it's safe to use the not-null assertion for mNote. Shouldn't we use the same approach as for the mComment?

In NotificationCommentDetailFragment, mNote should be non-null after onCreate().

I replaced with note here.

private val note: Note // note will be non-null after onCreate get() = mNote!!

justtwago · 2024-05-18T00:33:46Z

WordPress/src/main/java/org/wordpress/android/ui/engagement/BottomSheetUiState.kt

 sealed class BottomSheetUiState {
+    @Suppress("SerialVersionUIDInSerializableClass")
    data class UserProfileUiState(


I'm wondering if we shouldn't mark some fields as nullable because not all are always available for all the cases. For example, if the user does not have an avatar URL, it should be null but not an empty string that has less straight meaning. When using the state in different places with not enough context, it looks like the state has to contain all the data (except for onSiteClickListener.)
Just leaving this note for your consideration :)

I 100% agree with you, straight meaning is important for us to handle those fields correctly. I find many implementations use default values, but it'd be better if they are defined as nullable.

I suspect that when handling this data with Java in the past, there was a tendency to use defensive programming to avoid crashes. However, now with Kotlin, handling nullables has become much easier. I think we can find time to refactor this.

antonis · 2024-05-22T16:45:18Z

WordPress/src/main/java/org/wordpress/android/ui/comments/NotificationCommentDetailFragment.kt

@@ -16,6 +21,9 @@ import org.wordpress.android.util.ToastUtils
 * It'd be better to have multiple fragments for different sources for different purposes
 */
 class NotificationCommentDetailFragment : CommentDetailFragment() {
+    private val note: Note // note will be non-null after onCreate
+        get() = mNote!!


Wdyt of using this getter for all the mNote!! occurrences in this file?

Yeah, that makes sense, I missed some occurrences in this file when I made the change.

antonis · 2024-05-22T17:06:58Z

WordPress/src/main/java/org/wordpress/android/ui/comments/SiteCommentDetailFragment.kt

+    override fun getUserProfileUiState(): BottomSheetUiState.UserProfileUiState {
+        return BottomSheetUiState.UserProfileUiState(
+            userAvatarUrl = CommentExtension.getAvatarUrl(
+                mComment!!,


Nit pick: I wonder if we should avoid the non-null assertion operator for mComment in this file since it is assigned a nullable value. Wdyt of using ?. when possible with a default value?

Hmmm, I just reviewed the mComment, I have a strong sense that mComment should be non-null in SiteCommentDetailFragment, but the parent class CommentDetailFragment handles mComment with defensive programming and make it like nullable.

I plan to replace it with

private val comment: CommentModel get() = mComment!!

antonis

Great work @jarvislin 👍
I've tested the implementation on a Pixel 8 Pro (Android 14) and everything worked as described for me. The code also looks good and improves the codebase 🏅
I've left a few nit picks related to non-null assertions but nothing blocking on my side. We can revisit this separately as discussed

sonarcloud · 2024-05-24T13:30:50Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

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

See analysis details on SonarCloud

antonis

Thank you for the changes @jarvislin 🙇

jarvislin · 2024-05-24T14:53:00Z

Thank you both for reviewing this PR!

Jarvis Lin added 5 commits May 9, 2024 09:49

Update layouts and remove some redundant logic

8d88bfa

Ignore the replied footer on UI

5618cdf

Remove the action binding and related functions

79e0915

Fix lint issues

65a6bb0

Merge branch 'feature/notifications_refresh_p2' into issue/comment-de…

880e609

…tails-content # Conflicts: # WordPress/src/main/res/layout/comment_detail_fragment.xml

Jarvis Lin added 7 commits May 10, 2024 18:43

Fix some UI issues related to the header

348774e

Add a comment

5dfac69

Add a menu button in Comment Details

8c5d27b

Add the popup window

b0be301

Handle the Edit comment action

c02aace

Refactor the user profile bottom sheet

8ffe0f3

Refactor the codebase related to Comment Actions

2dd8b4a

jarvislin commented May 15, 2024

View reviewed changes

Reset binding to null when the view is destroyed

912a8d4

jarvislin commented May 15, 2024

View reviewed changes

Jarvis Lin added 7 commits May 15, 2024 17:42

Handle the comment action of User Info for the comment notifications

c660322

Display user info in the screen of My Site -> Comment List -> Comment

6070f47

Fix lint issues

5456a99

Rename a variable

2ab5cf0

Update a comment

d1bf95b

Update layout settings

a752f8a

Fix some RTL issues

381bb1d

jarvislin added Comments Notifications labels May 16, 2024

jarvislin requested a review from justtwago May 16, 2024 12:16

jarvislin marked this pull request as ready for review May 16, 2024 12:20

Fix a test

dc5cbe4

justtwago approved these changes May 18, 2024

View reviewed changes

Jarvis Lin added 2 commits May 20, 2024 13:21

Use orEmpty() to handle the empty strings

2b82a68

Use get() for handling a non-null variable

360d311

jarvislin requested a review from antonis May 22, 2024 07:19

antonis reviewed May 22, 2024

View reviewed changes

antonis approved these changes May 22, 2024

View reviewed changes

Jarvis Lin added 2 commits May 24, 2024 15:15

Replace mNote with note

d04ef21

Use get() to handle a non-null variable

1b18034

antonis approved these changes May 24, 2024

View reviewed changes

jarvislin merged commit afec253 into feature/notifications_refresh_p2 May 24, 2024
18 checks passed

jarvislin deleted the issue/comment-details-content branch May 24, 2024 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The redesign of comment details content #20798

The redesign of comment details content #20798

jarvislin commented May 10, 2024 •

edited

Loading

dangermattic commented May 10, 2024 •

edited

Loading

wpmobilebot commented May 10, 2024 •

edited

Loading

wpmobilebot commented May 10, 2024 •

edited

Loading

codecov bot commented May 10, 2024 •

edited

Loading

jarvislin May 15, 2024 •

edited

Loading

jarvislin May 15, 2024

jarvislin May 15, 2024

justtwago left a comment

justtwago May 18, 2024

jarvislin May 20, 2024 •

edited

Loading

justtwago May 18, 2024

jarvislin May 20, 2024

antonis May 22, 2024

jarvislin May 24, 2024

antonis May 22, 2024 •

edited

Loading

jarvislin May 24, 2024 •

edited

Loading

antonis left a comment

sonarcloud bot commented May 24, 2024

antonis left a comment

jarvislin commented May 24, 2024



		return BottomSheetUiState.UserProfileUiState(
		userAvatarUrl = mNote!!.iconURL,

The redesign of comment details content #20798

The redesign of comment details content #20798

Conversation

jarvislin commented May 10, 2024 • edited Loading

To Test:

Regression Notes

PR Submission Checklist:

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

dangermattic commented May 10, 2024 • edited Loading

wpmobilebot commented May 10, 2024 • edited Loading

wpmobilebot commented May 10, 2024 • edited Loading

codecov bot commented May 10, 2024 • edited Loading

Codecov Report

jarvislin May 15, 2024 • edited Loading

Choose a reason for hiding this comment

jarvislin May 15, 2024

Choose a reason for hiding this comment

jarvislin May 15, 2024

Choose a reason for hiding this comment

justtwago left a comment

Choose a reason for hiding this comment

justtwago May 18, 2024

Choose a reason for hiding this comment

jarvislin May 20, 2024 • edited Loading

Choose a reason for hiding this comment

justtwago May 18, 2024

Choose a reason for hiding this comment

jarvislin May 20, 2024

Choose a reason for hiding this comment

antonis May 22, 2024

Choose a reason for hiding this comment

jarvislin May 24, 2024

Choose a reason for hiding this comment

antonis May 22, 2024 • edited Loading

Choose a reason for hiding this comment

jarvislin May 24, 2024 • edited Loading

Choose a reason for hiding this comment

antonis left a comment

Choose a reason for hiding this comment

sonarcloud bot commented May 24, 2024

Quality Gate passed

antonis left a comment

Choose a reason for hiding this comment

jarvislin commented May 24, 2024

jarvislin commented May 10, 2024 •

edited

Loading

dangermattic commented May 10, 2024 •

edited

Loading

wpmobilebot commented May 10, 2024 •

edited

Loading

wpmobilebot commented May 10, 2024 •

edited

Loading

codecov bot commented May 10, 2024 •

edited

Loading

jarvislin May 15, 2024 •

edited

Loading

jarvislin May 20, 2024 •

edited

Loading

antonis May 22, 2024 •

edited

Loading

jarvislin May 24, 2024 •

edited

Loading