[Notifications Refresh] Comment Content Redesign #23147

salimbraksa · 2024-05-03T12:13:45Z

Fixes https://github.com/Automattic/wordpress-mobile/issues/31 and part of https://github.com/Automattic/wordpress-mobile/issues/38

Dependencies

This PR depends on:

[Notifications Refresh] Comment Moderation Content Header #23117

Description

This PR introduces a new component for the comment table view cell, while the previous cell, CommentContentTableViewCell, continues to be used in the Reader Comments screen. I decided against refactoring the original for a couple of reasons:

There's a risk of causing regression issues specifically on the Reader Comments screen.
The existing class is already quite complex and lengthy. Adding further logic to manage styling would complicate it even more.

The new class CommentDetailContentTableViewCell is comprised of 2 components:

CommentContentHeaderView: This component was implemented in this PR.
CommentDetailContentView: I've extracted the comment rendering logic from CommentContentTableViewCell into this component.

Comment	Menu

Test Instructions

Comment Moderation Allowed

Run Jetpack and login.
Navigate to "Notifications" screen.
Select a "Comment" notification.
Compare the implementation against the design.
If the comment has links or mentions, tap one of them and expect a web view is shown.
Tap the more menu.
Expect these buttons to be visible: "User Info", "Share", "Edit Comment" and "Change Status".
Verify "User Info", "Share" and "Edit Comment" buttons work as expected.

Comment Moderation Not Allowed

Go to Comment.swift, and force allowsModeration to return false.
Build and run the app.
Navigate to My Site > Comments.
Tap any comment.
Tap the more menu.
Expect these buttons to be visible: "User Info", "Share".

Regression Notes

Potential unintended areas of impact
Smoke test the "Comment Detail" screen when accessed from "My Site > Comments".
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing.
What automated tests I added (or what prevented me from doing so)
None.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing checklist:

wpmobilebot · 2024-05-03T12:27:33Z

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

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr23147-3d77d0a
	Version	24.8
	Bundle ID	org.wordpress.alpha
	Commit	`3d77d0a`
	App Center Build	WPiOS - One-Offs #9826

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2024-05-03T12:27:49Z

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

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr23147-3d77d0a
	Version	24.8
	Bundle ID	com.jetpack.alpha
	Commit	`3d77d0a`
	App Center Build	jetpack-installable-builds #8874

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

salimbraksa · 2024-05-03T17:34:38Z

WordPress/Classes/ViewRelated/Comments/Content/CommentDetailContentView.swift

@@ -0,0 +1,82 @@
+import UIKit
+
+final class CommentDetailContentView: UIView {


As I've mentioned in the PR description, most of the logic was extracted from CommentContentTableViewCell, except:

The cell was using a NSLayoutConstraint to constraint the height, I'm using intrinsicContentSize instead.

The cell was providing 2 methods to render comments ( web, richContent ). I've extracted only the web rendering method because it was the one used in Comment Detail screen.

Added a Configuration type to encapsulate configuration properties.

salimbraksa · 2024-05-03T17:36:17Z

WordPress/Classes/ViewRelated/Comments/Content/CommentDetailContentView.swift

+            case .rendered(let height): return height
+            }
+        }
+    }


This State enum is not very useful right now, but I was thinking of showing an activity indicator because the comment rendering is async.

salimbraksa · 2024-05-03T17:38:51Z

WordPress/Classes/ViewRelated/Comments/CommentDetailViewController.swift

-        )
-        button.accessibilityLabel = NSLocalizedString("Share comment", comment: "Accessibility label for button to share a comment from a notification")
-        return button
-    }()


Edit and share bar buttons were removed from the navigation bar, because now they're buttons in the comment's menu.

salimbraksa · 2024-05-03T17:39:44Z

WordPress/Classes/ViewRelated/Comments/CommentDetailViewController.swift

+            case .share:
+                shareCommentURL()
+            case .changeStatus(let status):
+                print("Option \(status) tapped")


Handling status changes will be addressed in a separate PR as it depends on this issue.

salimbraksa · 2024-05-03T17:45:12Z

WordPress/Classes/ViewRelated/Comments/CommentDetailViewController.swift

+        let contentConfig = CommentDetailContentTableViewCell.ContentConfiguration(comment: comment) { [weak self] _ in
+            UIView.performWithoutAnimation {
+                self?.tableView.performBatchUpdates({})
+            }


When rendering the comment, we use the performBatchUpdates method to relayout the cells and update their heights. This approach was part of the previous implementation. What I've added is the wrapping of this method inside UIView.performWithoutAnimation, ensuring that the cells' relayout occurs without any animation. In my opinion, the default animation appeared clunky.

salimbraksa · 2024-05-03T17:45:49Z

WordPress/Classes/ViewRelated/Comments/CommentDetailViewController.swift

@@ -729,7 +698,7 @@ private extension CommentDetailViewController {
        present(activityViewController, animated: true, completion: nil)
    }

-    func presentUserInfoSheet(_ senderView: UIView) {
+    func presentUserInfoSheet() {


The senderView: UIView param was not used, so I removed it.

justtwago

Tested the changes and everything works well. My only concern is a really little padding between a header and content. It looks really tight. Maybe it's worth double check with Chris and perhaps restore the previous value?

justtwago · 2024-05-06T21:27:57Z

WordPress/Classes/ViewRelated/Comments/Content/CommentDetailContentTableViewCell.swift

+    func configure(with headerConfig: HeaderConfiguration, contentConfig: ContentConfiguration, parent: UIViewController) {
+        self.updateHeader(with: headerConfig, parent: parent)
+        self.commentView.configure(with: contentConfig)
+    }


Just to double-check: what is the purpose of this initializer? It is not used and I'm not sure if we have a case for this.

This acts as the primary configure method, while the other one is merely a convenience method. The convenience configure method is supposed to call this one, but I just realized that it's not the case. Resolved in 3d77d0a

dangermattic · 2024-05-07T15:11:40Z

	1 Warning
⚠️	This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

salimbraksa added Notifications Jetpack App labels May 3, 2024

salimbraksa self-assigned this May 3, 2024

salimbraksa changed the base branch from trunk to task/notifications-comment-content-header May 3, 2024 12:14

salimbraksa force-pushed the task/notifications-comment-content branch from 888486a to 6f15c2b Compare May 3, 2024 12:19

salimbraksa commented May 3, 2024

View reviewed changes

salimbraksa requested review from alpavanoglu, justtwago and hassaanelgarem May 3, 2024 17:58

salimbraksa marked this pull request as ready for review May 3, 2024 17:58

Base automatically changed from task/notifications-comment-content-header to feature/notifications_refresh_p2 May 4, 2024 01:52

salimbraksa added 5 commits May 4, 2024 02:53

Integrate the new header in Comment Detail screen

269ef54

Extra comment content view component into its own class

1dd0800

Integrate the new comment view component

6beb84c

Handle user info, share and edit comment actions

02727e7

Fix app crashing when sharing comment URL using an iPad

95870d0

salimbraksa force-pushed the task/notifications-comment-content branch from 6a74476 to 95870d0 Compare May 4, 2024 01:53

justtwago reviewed May 6, 2024

View reviewed changes

salimbraksa added 2 commits May 7, 2024 15:43

Reset Swift Packages cache

7715cc2

Comment out notification UI tests until the redesign is complete

f623b9f

Minor code refactoring

3d77d0a

salimbraksa merged commit cac14dc into feature/notifications_refresh_p2 May 8, 2024
25 checks passed

salimbraksa deleted the task/notifications-comment-content branch May 8, 2024 10:05

salimbraksa mentioned this pull request May 10, 2024

[Notifications Refresh] Revert Packages.resolved changes #23188

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Notifications Refresh] Comment Content Redesign #23147

[Notifications Refresh] Comment Content Redesign #23147

salimbraksa commented May 3, 2024 •

edited

Loading

wpmobilebot commented May 3, 2024 •

edited

Loading

wpmobilebot commented May 3, 2024 •

edited

Loading

salimbraksa May 3, 2024 •

edited

Loading

salimbraksa May 3, 2024

salimbraksa May 3, 2024

salimbraksa May 3, 2024 •

edited

Loading

salimbraksa May 3, 2024

salimbraksa May 3, 2024

justtwago left a comment

justtwago May 6, 2024

salimbraksa May 7, 2024

dangermattic commented May 7, 2024

		@@ -0,0 +1,82 @@
		import UIKit

		final class CommentDetailContentView: UIView {

[Notifications Refresh] Comment Content Redesign #23147

[Notifications Refresh] Comment Content Redesign #23147

Conversation

salimbraksa commented May 3, 2024 • edited Loading

Dependencies

Description

Test Instructions

Comment Moderation Allowed

Comment Moderation Not Allowed

Regression Notes

PR submission checklist:

Testing checklist:

wpmobilebot commented May 3, 2024 • edited Loading

wpmobilebot commented May 3, 2024 • edited Loading

salimbraksa May 3, 2024 • edited Loading

Choose a reason for hiding this comment

salimbraksa May 3, 2024

Choose a reason for hiding this comment

salimbraksa May 3, 2024

Choose a reason for hiding this comment

salimbraksa May 3, 2024 • edited Loading

Choose a reason for hiding this comment

salimbraksa May 3, 2024

Choose a reason for hiding this comment

salimbraksa May 3, 2024

Choose a reason for hiding this comment

justtwago left a comment

Choose a reason for hiding this comment

justtwago May 6, 2024

Choose a reason for hiding this comment

salimbraksa May 7, 2024

Choose a reason for hiding this comment

dangermattic commented May 7, 2024

salimbraksa commented May 3, 2024 •

edited

Loading

wpmobilebot commented May 3, 2024 •

edited

Loading

wpmobilebot commented May 3, 2024 •

edited

Loading

salimbraksa May 3, 2024 •

edited

Loading

salimbraksa May 3, 2024 •

edited

Loading