[Notifications Refresh] Comment Moderation Content Header #23117

salimbraksa · 2024-04-29T23:41:10Z

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

This PR implements the header part of the comment content view. The rest of the view will addressed in a separate PR.

Header	Menu

Test Instructions

Go to WordPressAppDelegate.swift file.
Comment both didFinishLaunchingWithOptions and willFinishLaunchingWithOptions methods.
Go to CommentContentHeaderView.swift file.
Enable Previews.
Compare the implementation against the design.

Regression Notes

Potential unintended areas of impact
N/A
What I did to test those areas of impact (or what existing automated tests I relied on)
N/A
What automated tests I added (or what prevented me from doing so)
N/A

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:

salimbraksa · 2024-04-29T23:43:48Z

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

+import UIKit
+import SwiftUI
+
+final class CommentContentView: UIView {


This class is still WIP and will open another PR to finalize it.

wpmobilebot · 2024-04-29T23:57:05Z

📲 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	pr23117-81dd999
	Version	24.7
	Bundle ID	org.wordpress.alpha
	Commit	`81dd999`
	App Center Build	WPiOS - One-Offs #9780

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

wpmobilebot · 2024-04-29T23:57:23Z

📲 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	pr23117-81dd999
	Version	24.7
	Bundle ID	com.jetpack.alpha
	Commit	`81dd999`
	App Center Build	jetpack-installable-builds #8826

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

alpavanoglu · 2024-05-01T09:43:35Z

Modules/Sources/DesignSystem/Components/Content Preview/ContentPreview.swift

@@ -92,6 +92,7 @@ public struct ContentPreview: View {
    }
 }

+#if DEBUG


I think, this is redundant but won't cause any harm for sure. Compiler should be removing all the Previews and code that isn't used by the application on release version. So effectively, this should only be relevant to the developer and never to the AppStore version

Resolved in 17738c5

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

alpavanoglu · 2024-05-01T10:32:47Z

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

+        let avatarURL: URL?
+        let username: String
+        let handleAndTimestamp: String
+        let menu: [[MenuItem]]


I think it isn't very obvious why menu is an array of arrays. Could we improve the data structure to make the use case more explicit? Or perhaps rename it to highlight the sections & rows structure?

Resolved in c5ba22d

alpavanoglu · 2024-05-01T10:36:28Z

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

+                           button(for: menuItem)
+                       }
+                   }
+                   if sectionIndex < menu.count - 1 && !menu[sectionIndex + 1].isEmpty {


Could we improve this by using a compactMap or filter logic to remove sections which have no items? Then we could always place the divider after the last index available.

Resolved in 57e22b4

alpavanoglu · 2024-05-01T10:37:09Z

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

+           }
+       } label: {
+           Image.DS.icon(named: .ellipsisHorizontal)
+               .imageScale(.small)


This disables dynamic font sizing for the image. I am not sure if that's desired. I just wanted to highlight.

I removed that modifier and the icon still doesn't scale. I think this a known issue where custom icons don't scale like SF Symbols.

alpavanoglu · 2024-05-01T10:38:59Z

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

+           Button(action: action) {
+               Label {
+                   Text(Strings.userInfo)
+               } icon: {
+                   Image.DS.icon(named: .avatar)
+               }


I think we can extract this into a function that accepts a text and IconName and reduce the duplication.

Resolved in dd9db3f

alpavanoglu · 2024-05-01T10:41:42Z

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

+   enum Item {
+       case userInfo(() -> Void)
+       case share(() -> Void)
+       case editComment(() -> Void)
+       case changeStatus((Status) -> Void)
+
+       enum Status {
+           case approve, trash, spam, pending
+       }
+   }


Wouldn't a ViewModel be more scalable here? The view can report to actions there. Instead of dealing with closures, we the VM could implement all the cases. For 1 or 2 events I think closures are fine but this to me feels a bit rigid in terms of scalability. Wdyt?

I've tried having only 1 closure to report menu events, something like this:

enum Item { case userInfo case share case editComment case changeStatus(Status) enum Status { case approve, trash, spam, pending } }

Then I wanted to have a closure with this signature that callers will implement

let onItemSelected: (Item) -> Void = { item in ... }

The challenge arises with the .changeStatus(Status) case in our Item enum. When configuring the menu:

let menu = [.userInfo, .share, .editComment, .changeStatus(???)]

It's unclear how to specify a status for .changeStatus statically, as it represents multiple potential actions (approve, trash, spam, pending).

Can you share a pseudo-code of how the VM approach would work?

justtwago

Finally managed to test the previews and they look awesome! I like the sub-menu under the "Change status" section! ❤️
Looking forward to seeing this in practice.

justtwago

Thanks for the improvement. It's definitely clearer to have a separate menu configuration class. Great job!
Just left one minor Swift-related question.

justtwago · 2024-05-03T17:15:20Z

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

+    }
+
+    private var shouldShowMenu: Bool {
+        return [menu.userInfo, menu.share, menu.editComment, menu.changeStatus].reduce(false) { $0 || $1 }


Just curious if this function checks whether one of the menu items is true.
.reduce(false) { $0 || $1 } part seems to be not entirely intuitive, and I'm wondering if .contains { _ in true } would have a similar result.

Resolved in 7154ae5

salimbraksa added 4 commits April 30, 2024 00:35

Add edit and avatar icon

e09f77c

Add comment content header view

7bedf88

Add comment content view but it's still wip

05b72fd

Wrap SwiftUI preview logic in DEBUG macro

f0406af

salimbraksa added Notifications Jetpack App labels Apr 29, 2024

salimbraksa added this to the Pending milestone Apr 29, 2024

salimbraksa self-assigned this Apr 29, 2024

salimbraksa changed the base branch from trunk to feature/notifications_refresh_p2 April 29, 2024 23:42

salimbraksa commented Apr 29, 2024

View reviewed changes

salimbraksa requested a review from justtwago April 29, 2024 23:48

salimbraksa assigned alpavanoglu Apr 29, 2024

salimbraksa marked this pull request as ready for review April 29, 2024 23:48

salimbraksa assigned alpavanoglu and unassigned alpavanoglu Apr 29, 2024

salimbraksa requested a review from alpavanoglu April 29, 2024 23:55

Sync with base branch and fix conflicts

1bf164c

salimbraksa force-pushed the task/notifications-comment-content-header branch from 4624595 to 1bf164c Compare April 30, 2024 17:02

alpavanoglu reviewed May 1, 2024

View reviewed changes

salimbraksa added 5 commits May 1, 2024 11:58

Remove DEBUG macros from Previews

17738c5

Update code formatting

bd170ef

Improve menu data structure

c5ba22d

Extract menu button logic into a separate function

dd9db3f

Improve menu filtering logic

57e22b4

salimbraksa requested a review from alpavanoglu May 1, 2024 15:41

justtwago approved these changes May 2, 2024

View reviewed changes

Refactor Menu Configuration API

81dd999

salimbraksa mentioned this pull request May 3, 2024

[Notifications Refresh] Comment Content Redesign #23147

Merged

14 tasks

justtwago approved these changes May 3, 2024

View reviewed changes

Improve shouldShowMenu readability

7154ae5

salimbraksa merged commit 7924ea1 into feature/notifications_refresh_p2 May 4, 2024
3 of 18 checks passed

salimbraksa deleted the task/notifications-comment-content-header branch May 4, 2024 01:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Notifications Refresh] Comment Moderation Content Header #23117

[Notifications Refresh] Comment Moderation Content Header #23117

salimbraksa commented Apr 29, 2024 •

edited

Loading

salimbraksa Apr 29, 2024

wpmobilebot commented Apr 29, 2024 •

edited

Loading

wpmobilebot commented Apr 29, 2024 •

edited

Loading

alpavanoglu May 1, 2024

salimbraksa May 1, 2024

alpavanoglu May 1, 2024

salimbraksa May 1, 2024

alpavanoglu May 1, 2024

salimbraksa May 1, 2024

alpavanoglu May 1, 2024

salimbraksa May 1, 2024

alpavanoglu May 1, 2024

salimbraksa May 1, 2024

alpavanoglu May 1, 2024

salimbraksa May 1, 2024

salimbraksa May 1, 2024

justtwago left a comment

justtwago left a comment

justtwago May 3, 2024

salimbraksa May 4, 2024

[Notifications Refresh] Comment Moderation Content Header #23117

[Notifications Refresh] Comment Moderation Content Header #23117

Conversation

salimbraksa commented Apr 29, 2024 • edited Loading

Test Instructions

Regression Notes

PR submission checklist:

Choose a reason for hiding this comment

wpmobilebot commented Apr 29, 2024 • edited Loading

wpmobilebot commented Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justtwago left a comment

Choose a reason for hiding this comment

justtwago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salimbraksa commented Apr 29, 2024 •

edited

Loading

wpmobilebot commented Apr 29, 2024 •

edited

Loading

wpmobilebot commented Apr 29, 2024 •

edited

Loading