[Notifications P1] Replace Notifications Cell Content #22593

alpavanoglu · 2024-02-12T15:48:58Z

Description

Utilises the newly added NotificationsTableViewCellContent to replace the old cells used in NotificationsViewController.
Also contains improvements to AvatarsView like placeholder image and parsing multiple image urls from the Notification response.

Testing Steps

Install & login to Jetpack App
Navigate to Notifications tab.
✅ Verify that the UI matches the designs.
✅ Verify, cell selection does not highlight the cell.
✅ Verify various avatar styles: single, double, triple.
✅ Retry steps 3, 4 and 5 after toggling the theme color.

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:

dangermattic · 2024-02-12T15:50:16Z

	2 Warnings
⚠️	View files have been modified, but no screenshot is included in the pull request. Consider adding some for clarity.
⚠️	This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

WordPress/Classes/Models/Notifications/Notification.swift

wpmobilebot · 2024-02-12T15:57:48Z

📲 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	pr22593-c4657ca
	Version	24.2
	Bundle ID	org.wordpress.alpha
	Commit	`c4657ca`
	App Center Build	WPiOS - One-Offs #8789

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

wpmobilebot · 2024-02-12T15:58:00Z

📲 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	pr22593-c4657ca
	Version	24.2
	Bundle ID	com.jetpack.alpha
	Commit	`c4657ca`
	App Center Build	jetpack-installable-builds #7818

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

salimbraksa · 2024-02-12T22:32:28Z

WordPress/Classes/ViewRelated/Views/List/NotificationsList/HostingTableViewCell.swift

+    func host(_ view: Content, parent: UIViewController) {
+        if let controller = controller {
+            controller.rootView = view
+            controller.view.layoutIfNeeded()


[Discussion] I'm not sure if you ran into an issue where the cell doesn't resize to fit its content. From experience, the only method that worked for me is controller.view.invalidateIntrinsicContentSize.

There is another approach, but I haven't tried it yet:

tableView.selfSizingInvalidation = .enabledIncludingConstraints

Source https://appcircle.io/blog/using-swiftui-inside-uicollectionview-and-uitableview

I didn't notice any problems about the cell sizing. Did you?

Resolved ee81dd7

[Nit] layoutIfNeeded() is no longer needed when calling invalidateIntrinsicContentSize

Suggested change

controller.view.layoutIfNeeded()

salimbraksa · 2024-02-12T22:36:08Z

...ated/Notifications/Controllers/NotificationsViewController/NotificationsViewController.swift

+                )
+            ),
+            parent: self
+        )


[Question] I guess we don't handle the "Altered" state yet? Are you planning to work on that in this PR or a separate PR?

No I actually should've done it. I'll look into the old code now to see the conditions it is displayed.

Resolved: 2b1626e

salimbraksa · 2024-02-13T20:20:25Z

...ated/Notifications/Controllers/NotificationsViewController/NotificationsViewController.swift

+                            title: note.renderSubject()?.string ?? "",
+                            description: note.renderSnippet()?.string,
+                            shouldShowIndicator: !note.read,
+                            avatarStyle: .init(urls: note.allAvatarURLs) ?? .single(note.iconURL!),


.single(note.iconURL!)

[Suggestion] Honestly I wouldn't be confident force unwrapping a value that comes from the Backend. In my opinion, the avatarStyle can be nil, and in that case, we should display some placeholder.

Yeah we shouldn't force unwrap for sure. I'll fix it

Resolved 9abb1cc

salimbraksa · 2024-02-13T21:36:03Z

I've shared feedback with Alp internally because it included screenshots and recordings of the app showing information about the company.

salimbraksa · 2024-02-14T01:57:33Z

...ated/Notifications/Controllers/NotificationsViewController/NotificationsViewController.swift

+            return UITableViewCell()
+        }
+        cell.selectionStyle = .none
+        if let deletionRequest = notificationDeletionRequests[note.objectID] {


[Nit] This if-else is a little bit hard to read, also, the cell.host method call is redundant. Why not do this if-else logic only for the style computation, because it is the only dynamic part in this logic?

let style = { if let deletionRequest { // return altered style } // return regular style }() cell.host(...)

Resolved 9abb1cc

WordPress/Classes/ViewRelated/Views/List/NotificationsList/HostingTableViewCell.swift

salimbraksa

All good 🚀

alpavanoglu added 10 commits February 12, 2024 15:27

Add HostingTableViewCell

382c489

Update header bottom padding

34b8640

Remove namespacing on NotificationsTableViewCellContent

2251d1b

Increase padding between texts

5d699d0

Add allAvatarURLs parsing

2939732

Update cell height calculation

47e3072

Add placeholder image for AvatarsView

ac5ad17

Clean up HostingTableViewController

f786698

Add reuseIdentifier constant

2795514

Update tableview.selectionStyle

93a011c

alpavanoglu added Notifications [Type] Task labels Feb 12, 2024

alpavanoglu added this to the 24.3 milestone Feb 12, 2024

alpavanoglu requested a review from salimbraksa February 12, 2024 15:48

alpavanoglu commented Feb 12, 2024

View reviewed changes

WordPress/Classes/Models/Notifications/Notification.swift Show resolved Hide resolved

salimbraksa reviewed Feb 12, 2024

View reviewed changes

alpavanoglu added 2 commits February 13, 2024 16:36

Add test case for allAvatarURLs

080840b

Add deleted cell logic

2b1626e

salimbraksa reviewed Feb 13, 2024

View reviewed changes

salimbraksa self-requested a review February 13, 2024 21:35

salimbraksa reviewed Feb 14, 2024

View reviewed changes

alpavanoglu added 3 commits February 14, 2024 17:24

Fix triple avatar cell vertical alignment

0ee88c7

Add invalidateIntrinsicContentSize to HostingTableViewCell

ee81dd7

Update test json to fix test

7aafea5

salimbraksa reviewed Feb 14, 2024

View reviewed changes

WordPress/Classes/ViewRelated/Views/List/NotificationsList/HostingTableViewCell.swift Outdated Show resolved Hide resolved

salimbraksa reviewed Feb 14, 2024

View reviewed changes

WordPress/Classes/ViewRelated/Views/List/NotificationsList/HostingTableViewCell.swift Outdated Show resolved Hide resolved

alpavanoglu added 4 commits February 14, 2024 17:47

Remove force-unwrap & reduce boilerplate in if-else

9abb1cc

Remove redundant layout calls

49367c1

Replace spacers with padding on AvatarsView

8859fcf

Replace title as AttributedText to allow reply icon to appear

c4657ca

alpavanoglu requested a review from salimbraksa February 14, 2024 18:17

salimbraksa approved these changes Feb 14, 2024

View reviewed changes

salimbraksa merged commit 37a3215 into feature/notifications_refresh_p1 Feb 14, 2024
21 of 22 checks passed

salimbraksa deleted the feature/notifications-adopt-new-cell-content branch February 14, 2024 18:51

pinarol mentioned this pull request Apr 1, 2024

[Gravatar] Gravatar SDK integration umbrella task #22543

Closed

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Notifications P1] Replace Notifications Cell Content #22593

[Notifications P1] Replace Notifications Cell Content #22593

alpavanoglu commented Feb 12, 2024

dangermattic commented Feb 12, 2024 •

edited

Loading

wpmobilebot commented Feb 12, 2024 •

edited

Loading

wpmobilebot commented Feb 12, 2024 •

edited

Loading

salimbraksa Feb 12, 2024

alpavanoglu Feb 13, 2024

alpavanoglu Feb 14, 2024

salimbraksa Feb 14, 2024 •

edited

Loading

salimbraksa Feb 12, 2024

alpavanoglu Feb 13, 2024

alpavanoglu Feb 13, 2024

salimbraksa Feb 13, 2024

alpavanoglu Feb 14, 2024

alpavanoglu Feb 14, 2024

salimbraksa commented Feb 13, 2024 •

edited

Loading

salimbraksa Feb 14, 2024 •

edited

Loading

alpavanoglu Feb 14, 2024

salimbraksa left a comment

[Notifications P1] Replace Notifications Cell Content #22593

[Notifications P1] Replace Notifications Cell Content #22593

Conversation

alpavanoglu commented Feb 12, 2024

Description

Testing Steps

Regression Notes

dangermattic commented Feb 12, 2024 • edited Loading

wpmobilebot commented Feb 12, 2024 • edited Loading

wpmobilebot commented Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salimbraksa Feb 14, 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

salimbraksa commented Feb 13, 2024 • edited Loading

salimbraksa Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salimbraksa left a comment

Choose a reason for hiding this comment

dangermattic commented Feb 12, 2024 •

edited

Loading

wpmobilebot commented Feb 12, 2024 •

edited

Loading

wpmobilebot commented Feb 12, 2024 •

edited

Loading

salimbraksa Feb 14, 2024 •

edited

Loading

salimbraksa commented Feb 13, 2024 •

edited

Loading

salimbraksa Feb 14, 2024 •

edited

Loading