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.

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

[Notification Refresh] Likes and follows notifications header redesign #23107

Merged
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions Modules/Sources/DesignSystem/Foundation/IconName.swift
Expand Up @@ -19,6 +19,8 @@ public enum IconName: String, CaseIterable {
case starOutline = "star.outline"
case chevronRight = "chevron.right"
case exclamationCircle = "exclamation.circle"
case arrowUp = "arrow.up"
case arrowDown = "arrow.down"
}

// MARK: - Load Image
Expand Down
@@ -0,0 +1,12 @@
{
"images" : [
{
"filename" : "arrow-down.pdf",
"idiom" : "universal"
}
],
"info" : {
"author" : "xcode",
"version" : 1
}
}
Binary file not shown.
@@ -0,0 +1,12 @@
{
"images" : [
{
"filename" : "arrow-up.pdf",
"idiom" : "universal"
}
],
"info" : {
"author" : "xcode",
"version" : 1
}
}
Binary file not shown.
41 changes: 27 additions & 14 deletions WordPress/Classes/ViewRelated/Likes/LikesListController.swift
Expand Up @@ -22,6 +22,7 @@ import WordPressKit

class LikesListController: NSObject {

private let parent: UIViewController
Copy link
Contributor

Choose a reason for hiding this comment

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

[Suggestion] if the view controller owns this LikesListController, that in turns owns the view controller through the parent property, then this is a reference cycle and it will cause a memory leak. I'd suggest to mark parent as weak.

Then you can safe unwrap the optional parent property when configuring the cells:

guard let parent else {
   return
}
cell.configure(..., parent: parent)

If the parent is nil, that means the view controller was deallocated and the screen is no longer visible to the user.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch! Didn't notice the looped references.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it is ensured semantically that Child can only exist when Parent exists, you can also use unowned that removes the need to unwrap. It is kind of a forced unwrapped weak reference in practice. However it highlights the relationship between the 2 objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I prefer using weak in all scenarios, as using unowned carries the risk of the app crashing if the child’s lifecycle outlives that of its parent. In other word, if parent is deallocated and the child attempts to access it, the app will crash.

I'm okey with keeping unowned, but I just wanted to let you know of the potential risks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I researched the difference between weak and unowned modifiers, and I assume that LikesListController cannot naturally exist without the "parent" NotificationDetailsViewController. Such an incorrect state would lead to crashes and defects by default despite this value. I agree that using this modifier, in this case, more straightly indicates a cycled dependency, which is, in my opinion, more beneficial.

private let formatter = FormattableContentFormatter()
private let content: ContentIdentifier
private let siteID: NSNumber
Expand Down Expand Up @@ -87,7 +88,7 @@ class LikesListController: NSObject {

/// Init with Notification
///
init?(tableView: UITableView, notification: Notification, delegate: LikesListControllerDelegate? = nil) {
init?(tableView: UITableView, notification: Notification, parent: UIViewController, delegate: LikesListControllerDelegate? = nil) {

guard let siteID = notification.metaSiteID else {
return nil
Expand All @@ -113,6 +114,7 @@ class LikesListController: NSObject {
return nil
}

self.parent = parent
self.notification = notification
self.siteID = siteID
self.tableView = tableView
Expand All @@ -124,7 +126,7 @@ class LikesListController: NSObject {

/// Init with ReaderPost
///
init?(tableView: UITableView, post: ReaderPost, delegate: LikesListControllerDelegate? = nil) {
init?(tableView: UITableView, post: ReaderPost, parent: UIViewController, delegate: LikesListControllerDelegate? = nil) {

guard let postID = post.postID else {
return nil
Expand All @@ -133,6 +135,7 @@ class LikesListController: NSObject {
content = .post(id: postID)
readerPost = post
siteID = post.siteID
self.parent = parent
self.tableView = tableView
self.delegate = delegate

Expand Down Expand Up @@ -339,11 +342,6 @@ extension LikesListController: UITableViewDataSource, UITableViewDelegate {
func tableView(_ tableView: UITableView, didSelectRowAt indexPath: IndexPath) {
tableView.deselectRow(at: indexPath, animated: true)

if showingNotificationLikes && indexPath.section == Constants.headerSectionIndex {
delegate?.didSelectHeader?()
return
}

guard !isLoadingContent,
let user = likingUsers[safe: indexPath.row] else {
return
Expand All @@ -370,20 +368,35 @@ private extension LikesListController {
}

func setupHeaderCell(cell: NoteBlockHeaderTableViewCell, group: FormattableContentGroup) {
cell.attributedHeaderTitle = nil
cell.attributedHeaderDetails = nil

guard let gravatarBlock: NotificationTextContent = group.blockOfKind(.image),
let snippetBlock: NotificationTextContent = group.blockOfKind(.text) else {
return
}

cell.attributedHeaderTitle = formatter.render(content: gravatarBlock, with: HeaderContentStyles())
cell.attributedHeaderDetails = formatter.render(content: snippetBlock, with: HeaderDetailsContentStyles())

// Download the Gravatar
let mediaURL = gravatarBlock.media.first?.mediaURL
cell.downloadAuthorAvatar(with: mediaURL)
let content = snippetBlock.text ?? ""
let action: () -> Void = { [weak self] in self?.delegate?.didSelectHeader!() }

if notification?.kind == .commentLike || notification?.kind == .follow {
let avatar = NoteBlockHeaderTableViewCell.Avatar(
url: mediaURL,
email: nil,
size: NoteBlockHeaderTableViewCell.Constants.imageSize
)
cell.configure(
avatar: avatar,
comment: content,
action: action,
parent: parent
)
} else {
cell.configure(
post: content,
action: action,
parent: parent
)
}
}
justtwago marked this conversation as resolved.
Show resolved Hide resolved

func userCell(for indexPath: IndexPath) -> UITableViewCell {
Expand Down
Expand Up @@ -340,12 +340,12 @@ extension NotificationDetailsViewController {

navigationItem.backBarButtonItem = backButton

let next = UIButton(type: .custom)
next.setImage(.gridicon(.arrowUp), for: .normal)
let next = UIButton(type: .system)
next.setImage(UIImage.DS.icon(named: .arrowUp), for: .normal)
next.addTarget(self, action: #selector(nextNotificationWasPressed), for: .touchUpInside)

let previous = UIButton(type: .custom)
previous.setImage(.gridicon(.arrowDown), for: .normal)
let previous = UIButton(type: .system)
previous.setImage(UIImage.DS.icon(named: .arrowDown), for: .normal)
previous.addTarget(self, action: #selector(previousNotificationWasPressed), for: .touchUpInside)

previousNavigationButton = previous
Expand Down Expand Up @@ -394,7 +394,7 @@ extension NotificationDetailsViewController {
/// since notification kind may change.
func setupTableDelegates() {
if note.kind == .like || note.kind == .commentLike,
let likesListController = LikesListController(tableView: tableView, notification: note, delegate: self) {
let likesListController = LikesListController(tableView: tableView, notification: note, parent: self, delegate: self) {
tableView.delegate = likesListController
tableView.dataSource = likesListController
self.likesListController = likesListController
Expand Down Expand Up @@ -621,20 +621,35 @@ private extension NotificationDetailsViewController {
// - UITableViewCell's taps don't require a Gestures Recognizer. No big deal, but less code!
//

cell.attributedHeaderTitle = nil
cell.attributedHeaderDetails = nil

guard let gravatarBlock: NotificationTextContent = blockGroup.blockOfKind(.image),
let snippetBlock: NotificationTextContent = blockGroup.blockOfKind(.text) else {
return
}

cell.attributedHeaderTitle = formatter.render(content: gravatarBlock, with: HeaderContentStyles())
cell.attributedHeaderDetails = formatter.render(content: snippetBlock, with: HeaderDetailsContentStyles())

// Download the Gravatar
let mediaURL = gravatarBlock.media.first?.mediaURL
cell.downloadAuthorAvatar(with: mediaURL)
let content = snippetBlock.text ?? ""
let action: () -> Void = { [weak self] in self?.didSelectHeader() }

if note?.kind == .commentLike || note?.kind == .follow {
let avatar = NoteBlockHeaderTableViewCell.Avatar(
url: mediaURL,
email: nil,
salimbraksa marked this conversation as resolved.
Show resolved Hide resolved
size: NoteBlockHeaderTableViewCell.Constants.imageSize
)
cell.configure(
avatar: avatar,
comment: content,
action: action,
parent: self
)
} else {
cell.configure(
post: content,
action: action,
parent: self
)
}
}

func setupFooterCell(_ cell: NoteBlockTextTableViewCell, blockGroup: FormattableContentGroup) {
Expand Down
Expand Up @@ -2,97 +2,57 @@ import Foundation
import WordPressShared.WPStyleGuide
import WordPressUI
import Gravatar
import SwiftUI
import DesignSystem

// MARK: - NoteBlockHeaderTableViewCell
//
class NoteBlockHeaderTableViewCell: NoteBlockTableViewCell {
typealias Constants = ContentPreview.Constants
typealias Avatar = ContentPreview.ImageConfiguration.Avatar

// MARK: - Private
private var authorAvatarURL: URL?
private typealias Style = WPStyleGuide.Notifications
private var controller: UIHostingController<HeaderView>?

// MARK: - IBOutlets
@IBOutlet private var authorAvatarImageView: UIImageView!
@IBOutlet private var headerTitleLabel: UILabel!
@IBOutlet private var headerDetailsLabel: UILabel!

// MARK: - Public Properties
@objc var headerTitle: String? {
set {
headerTitleLabel.text = newValue
}
get {
return headerTitleLabel.text
}
}

@objc var attributedHeaderTitle: NSAttributedString? {
set {
headerTitleLabel.attributedText = newValue
}
get {
return headerTitleLabel.attributedText
}
}

@objc var headerDetails: String? {
set {
headerDetailsLabel.text = newValue
}
get {
return headerDetailsLabel.text
}
func configure(post: String, action: @escaping () -> Void, parent: UIViewController) {
let content = ContentPreview(text: post, action: action)
host(HeaderView(preview: content), parent: parent)
}

@objc var attributedHeaderDetails: NSAttributedString? {
set {
headerDetailsLabel.attributedText = newValue
}
get {
return headerDetailsLabel.attributedText
}
func configure(avatar: Avatar, comment: String, action: @escaping () -> Void, parent: UIViewController) {
let content = ContentPreview(image: .init(avatar: avatar), text: comment, action: action)
host(HeaderView(preview: content), parent: parent)
}

// MARK: - Public Methods

@objc(downloadAuthorAvatarWithURL:)
func downloadAuthorAvatar(with url: URL?) {
guard url != authorAvatarURL else {
return
}
private func host(_ content: HeaderView, parent: UIViewController) {
if let controller = controller {
controller.rootView = content
controller.view.layoutIfNeeded()
} else {
let cellViewController = UIHostingController(rootView: content)
controller = cellViewController

authorAvatarURL = url
parent.addChild(cellViewController)
contentView.addSubview(cellViewController.view)
cellViewController.view.translatesAutoresizingMaskIntoConstraints = false
layout(hostingView: cellViewController.view)

guard let url = url else {
authorAvatarImageView.image = .gravatarPlaceholderImage
return
}

if let gravatar = AvatarURL(url: url) {
authorAvatarImageView.downloadGravatar(gravatar, placeholder: .gravatarPlaceholderImage, animate: true)
} else {
authorAvatarImageView.downloadSiteIcon(at: url.absoluteString)
cellViewController.didMove(toParent: parent)
}
}

// MARK: - View Methods

override func awakeFromNib() {
super.awakeFromNib()
func layout(hostingView view: UIView) {
self.contentView.pinSubviewToAllEdges(view)
}
}
justtwago marked this conversation as resolved.
Show resolved Hide resolved

accessoryType = .disclosureIndicator
backgroundColor = Style.blockBackgroundColor
private struct HeaderView: View {
private let preview: ContentPreview

headerTitleLabel.font = Style.headerTitleBoldFont
headerTitleLabel.textColor = Style.headerTitleColor
headerDetailsLabel.font = Style.headerDetailsRegularFont
headerDetailsLabel.textColor = Style.headerDetailsColor
authorAvatarImageView.image = .gravatarPlaceholderImage
public init(preview: ContentPreview) {
self.preview = preview
}

// MARK: - Overriden Methods
override func refreshSeparators() {
separatorsView.bottomVisible = true
separatorsView.bottomInsets = UIEdgeInsets.zero
var body: some View {
preview.padding(EdgeInsets(top: 16.0, leading: 16.0, bottom: 8.0, trailing: 16.0))
}
}