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.

Sign up for GitHub

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

[Notifications P1] Replace Notifications Cell Content #22593

Merged
merged 19 commits into from
Feb 14, 2024
Merged

[Notifications P1] Replace Notifications Cell Content #22593

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
382c489
Add `HostingTableViewCell`
alpavanoglu Feb 12, 2024
34b8640
Update header bottom padding
alpavanoglu Feb 12, 2024
2251d1b
Remove namespacing on `NotificationsTableViewCellContent`
alpavanoglu Feb 12, 2024
5d699d0
Increase padding between texts
alpavanoglu Feb 12, 2024
2939732
Add `allAvatarURLs` parsing
alpavanoglu Feb 12, 2024
47e3072
Update cell height calculation
alpavanoglu Feb 12, 2024
ac5ad17
Add placeholder image for `AvatarsView`
alpavanoglu Feb 12, 2024
f786698
Clean up `HostingTableViewController`
alpavanoglu Feb 12, 2024
2795514
Add `reuseIdentifier` constant
alpavanoglu Feb 12, 2024
93a011c
Update tableview.selectionStyle
alpavanoglu Feb 12, 2024
080840b
Add test case for `allAvatarURLs`
alpavanoglu Feb 13, 2024
2b1626e
Add deleted cell logic
alpavanoglu Feb 13, 2024
0ee88c7
Fix triple avatar cell vertical alignment
alpavanoglu Feb 14, 2024
ee81dd7
Add `invalidateIntrinsicContentSize` to `HostingTableViewCell`
alpavanoglu Feb 14, 2024
7aafea5
Update test json to fix test
alpavanoglu Feb 14, 2024
9abb1cc
Remove force-unwrap & reduce boilerplate in if-else
alpavanoglu Feb 14, 2024
49367c1
Remove redundant layout calls
alpavanoglu Feb 14, 2024
8859fcf
Replace spacers with padding on `AvatarsView`
alpavanoglu Feb 14, 2024
c4657ca
Replace `title` as `AttributedText` to allow reply icon to appear
alpavanoglu Feb 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions WordPress/Classes/Models/Notifications/Notification.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,21 @@ extension Notification {
}
return content.last
}

var allAvatarURLs: [URL] {
salimbraksa marked this conversation as resolved.
Show resolved Hide resolved
let firstMedias: [AnyObject] = body?.compactMap {
let allMedia = $0["media"] as? [AnyObject]
return allMedia?.first as? AnyObject
} ?? []

let urlStrings = firstMedias.compactMap {
$0["url"] as? String
}

return urlStrings.compactMap {
URL(string: $0)
}
}
}

// MARK: - Update Helpers
Expand Down
2 changes: 1 addition & 1 deletion .../Notifications/Controllers/NotificationsViewController/NotificationsTableHeaderView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ final class NotificationsTableHeaderView: UITableViewHeaderFooterView {
static let layoutMargins = NSDirectionalEdgeInsets(
top: Length.Padding.single,
leading: Length.Padding.double,
bottom: Length.Padding.single,
bottom: Length.Padding.double,
trailing: Length.Padding.double
)
}
Expand Down
46 changes: 43 additions & 3 deletions ...d/Notifications/Controllers/NotificationsViewController/NotificationsViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import WordPressAuthenticator
import Gridicons
import UIKit
import WordPressUI
import SwiftUI

/// The purpose of this class is to render the collection of Notifications, associated to the main
/// WordPress.com account.
Expand Down Expand Up @@ -331,8 +332,43 @@ class NotificationsViewController: UIViewController, UIViewControllerRestoration
}

func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
let cell = tableView.dequeueReusableCell(withIdentifier: ListTableViewCell.defaultReuseID, for: indexPath)
configureCell(cell, at: indexPath)
guard let cell = tableView.dequeueReusableCell(withIdentifier: NotificationsTableViewCellContent.reuseIdentifier) as? HostingTableViewCell<NotificationsTableViewCellContent>,
let note = tableViewHandler.resultsController?.managedObject(atUnsafe: indexPath) as? Notification else {
return UITableViewCell()
}
cell.selectionStyle = .none
if let deletionRequest = notificationDeletionRequests[note.objectID] {
Copy link
Contributor

@salimbraksa salimbraksa Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

[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(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved 9abb1cc

cell.host(
NotificationsTableViewCellContent(
style: .altered(
.init(
text: deletionRequest.kind.legendText,
action: { [weak self] in
self?.cancelDeletionRequestForNoteWithID(note.objectID)
}
)
)
),
parent: self
)
} else {
cell.host(
NotificationsTableViewCellContent(
style: .regular(
.init(
title: note.renderSubject()?.string ?? "",
description: note.renderSnippet()?.string,
shouldShowIndicator: !note.read,
avatarStyle: .init(urls: note.allAvatarURLs) ?? .single(note.iconURL!),
Copy link
Contributor

Choose a reason for hiding this comment

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

 .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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved 9abb1cc

actionIconName: nil
)
)
),
parent: self
)
}

cell.accessibilityHint = Self.accessibilityHint(for: note)
return cell
}

Expand Down Expand Up @@ -588,13 +624,17 @@ private extension NotificationsViewController {
func setupTableView() {
// Register the cells
tableView.register(NotificationsTableHeaderView.self, forHeaderFooterViewReuseIdentifier: NotificationsTableHeaderView.reuseIdentifier)
tableView.register(ListTableViewCell.defaultNib, forCellReuseIdentifier: ListTableViewCell.defaultReuseID)
tableView.register(
HostingTableViewCell<NotificationsTableViewCellContent>.self,
forCellReuseIdentifier: NotificationsTableViewCellContent.reuseIdentifier
)

// UITableView
tableView.accessibilityIdentifier = "notifications-table"
tableView.cellLayoutMarginsFollowReadableWidth = false
tableView.estimatedSectionHeaderHeight = UITableView.automaticDimension
tableView.backgroundColor = .systemBackground
tableView.separatorStyle = .none
view.backgroundColor = .systemBackground
WPStyleGuide.configureAutomaticHeightRows(for: tableView)
}
Expand Down
6 changes: 3 additions & 3 deletions WordPress/Classes/ViewRelated/Views/List/NotificationsList/AvatarsView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import WordPressUI
struct AvatarsView: View {
private enum Constants {
static let doubleAvatarHorizontalOffset: CGFloat = 18
static let tripleAvatarViewHeight: CGFloat = 44
}

enum Style {
Expand Down Expand Up @@ -74,7 +73,8 @@ struct AvatarsView: View {
return AsyncImage(url: processedURL) { image in
image.resizable()
} placeholder: {
borderColor
Image("gravatar")
.resizable()
}
.frame(width: style.diameter, height: style.diameter)
.clipShape(Circle())
Expand Down Expand Up @@ -116,7 +116,7 @@ struct AvatarsView: View {
.avatarBorderOverlay()
}
}
.frame(height: Constants.tripleAvatarViewHeight)
.frame(height: Length.Padding.large + style.diameter)
}
}

Expand Down
27 changes: 27 additions & 0 deletions WordPress/Classes/ViewRelated/Views/List/NotificationsList/HostingTableViewCell.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import UIKit
import SwiftUI

class HostingTableViewCell<Content: View>: UITableViewCell {
private weak var controller: UIHostingController<Content>?

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

Choose a reason for hiding this comment

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

[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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved ee81dd7

Copy link
Contributor

@salimbraksa salimbraksa Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Suggested change
controller.view.layoutIfNeeded()

} else {
let swiftUICellViewController = UIHostingController(rootView: view)
controller = swiftUICellViewController
swiftUICellViewController.view.backgroundColor = .clear

layoutIfNeeded()
alpavanoglu marked this conversation as resolved.
Show resolved Hide resolved

parent.addChild(swiftUICellViewController)
contentView.addSubview(swiftUICellViewController.view)
swiftUICellViewController.view.translatesAutoresizingMaskIntoConstraints = false
contentView.pinSubviewToAllEdges(swiftUICellViewController.view)

swiftUICellViewController.didMove(toParent: parent)
swiftUICellViewController.view.layoutIfNeeded()
alpavanoglu marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
4 changes: 0 additions & 4 deletions WordPress/Classes/ViewRelated/Views/List/NotificationsList/NotificationsTableViewCell.swift
View file
Open in desktop

This file was deleted.

89 changes: 50 additions & 39 deletions .../Classes/ViewRelated/Views/List/NotificationsList/NotificationsTableViewCellContent.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,46 +1,46 @@
import SwiftUI
import DesignSystem

extension NotificationsTableViewCell {
struct Content: View {
enum Style {
struct Regular {
let title: String
let description: String?
let shouldShowIndicator: Bool
let avatarStyle: AvatarsView.Style
let actionIconName: String? // TODO: Will be refactored to contain the action.
}

struct Altered {
let text: String
let actionTitle: String
let action: (() -> Void)?
}
struct NotificationsTableViewCellContent: View {
static let reuseIdentifier = String(describing: Self.self)
enum Style {
struct Regular {
let title: String
let description: String?
let shouldShowIndicator: Bool
let avatarStyle: AvatarsView.Style
let actionIconName: String? // TODO: Will be refactored to contain the action.
}

case regular(Regular)
case altered(Altered)
struct Altered {
let text: String
let action: (() -> Void)?
}

private let style: Style
case regular(Regular)
case altered(Altered)
}

init(style: Style) {
self.style = style
}
private let style: Style

var body: some View {
switch style {
case .regular(let regular):
Regular(info: regular)
case .altered(let altered):
Altered(info: altered)
}
init(style: Style) {
self.style = style
}

var body: some View {
switch style {
case .regular(let regular):
Regular(info: regular)
.padding(.bottom, Length.Padding.medium)
case .altered(let altered):
Altered(info: altered)
.padding(.bottom, Length.Padding.medium)
}
}
}

// MARK: - Regular Style
fileprivate extension NotificationsTableViewCell.Content {
fileprivate extension NotificationsTableViewCellContent {
struct Regular: View {
private let info: Style.Regular

Expand All @@ -57,6 +57,7 @@ fileprivate extension NotificationsTableViewCell.Content {
if let actionIconName = info.actionIconName {
actionIcon(withName: actionIconName)
}
Spacer()
}
.padding(.trailing, Length.Padding.double)
}
Expand All @@ -83,7 +84,7 @@ fileprivate extension NotificationsTableViewCell.Content {
}

private var textsVStack: some View {
VStack(alignment: .leading, spacing: 2) {
VStack(alignment: .leading, spacing: Length.Padding.half) {
Text(info.title)
.style(.bodySmall(.regular))
.foregroundStyle(Color.DS.Foreground.primary)
Expand All @@ -108,7 +109,17 @@ fileprivate extension NotificationsTableViewCell.Content {
}

// MARK: - Regular Style
fileprivate extension NotificationsTableViewCell.Content {
fileprivate extension NotificationsTableViewCellContent {
private enum Strings {
static let undoButtonText = NSLocalizedString(
"Undo",
comment: "Revert an operation"
)
static let undoButtonHint = NSLocalizedString(
"Reverts the action performed on this notification.",
comment: "Accessibility hint describing what happens if the undo button is tapped."
)
}
struct Altered: View {
private let info: Style.Altered

Expand All @@ -132,10 +143,11 @@ fileprivate extension NotificationsTableViewCell.Content {
Button(action: {
info.action?()
}, label: {
Text(info.actionTitle)
Text(Strings.undoButtonText)
.style(.bodySmall(.regular))
.foregroundStyle(Color.white)
.padding(.trailing, Length.Padding.medium)
.accessibilityHint(Strings.undoButtonHint)
.padding(.trailing, Length.Padding.medium)
})
}
}
Expand All @@ -148,7 +160,7 @@ fileprivate extension NotificationsTableViewCell.Content {
#if DEBUG
#Preview {
VStack(alignment: .leading, spacing: Length.Padding.medium) {
NotificationsTableViewCell.Content(
NotificationsTableViewCellContent(
style: .regular(
.init(
title: "John Smith liked your comment more than all other comments as asdf",
Expand All @@ -162,7 +174,7 @@ fileprivate extension NotificationsTableViewCell.Content {
)
)

NotificationsTableViewCell.Content(
NotificationsTableViewCellContent(
style: .regular(
.init(
title: "Albert Einstein and Marie Curie liked your comment on Quantum Mechanical solution for Hydrogen",
Expand All @@ -176,7 +188,7 @@ fileprivate extension NotificationsTableViewCell.Content {
)
)
)
NotificationsTableViewCell.Content(
NotificationsTableViewCellContent(
style: .regular(
.init(
title: "New likes on Night Time in Tokyo",
Expand All @@ -192,11 +204,10 @@ fileprivate extension NotificationsTableViewCell.Content {
)
)

NotificationsTableViewCell.Content(
NotificationsTableViewCellContent(
style: .altered(
.init(
text: "Comment has been marked as Spam",
actionTitle: "Undo",
action: nil
)
)
Expand Down
Loading