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
[Notification Refresh] Likes and follows notifications header redesign #23107
Conversation
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
|
The likes header view is updated accordingly to the new content preview config
WordPress/Classes/ViewRelated/Notifications/Controllers/NotificationDetailsViewController.swift
Outdated
Show resolved
Hide resolved
WordPress/Classes/ViewRelated/Notifications/Views/NoteBlockHeaderTableViewCell.swift
Show resolved
Hide resolved
WordPress/Classes/ViewRelated/Notifications/Controllers/NotificationDetailsViewController.swift
Show resolved
Hide resolved
@@ -22,6 +22,7 @@ import WordPressKit | |||
|
|||
class LikesListController: NSObject { | |||
|
|||
private let parent: UIViewController |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…g looped reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
b3f73c3
to
b3d1c8c
Compare
Fixes https://github.com/Automattic/wordpress-mobile/issues/16
This PR contains only the brand-new header UI and UX.
To test:
Regression Notes
Potential unintended areas of impact
Header navigation buttons and the original content panel actions.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual tests.
What automated tests I added (or what prevented me from doing so)
There have been no tests since the legacy code was changed only.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.Testing checklist: