Notifications > Comment Details: show previous/next comments

[ScoutHarris]

Ref: #17790

In the Comment Notifications details view, the previous and next buttons now display the previous and next comments.

Note: this only works in the Comments filter. The All and Unread filters have not been updated yet.

To test:

• Enable the notificationCommentDetails feature.
• Go to Notifications > Comments.
• Select a notification.
• Tap the previous and next arrows in the nav bar.
• Verify the previous and next comments are displayed.

Regression Notes

1. Potential unintended areas of impact
   N/A. Feature is disabled.

2. What I did to test those areas of impact (or what existing automated tests I relied on)
   N/A

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

[jkmassel]

You can test the Jetpack changes on this Pull Request by downloading it from AppCenter here with build number: pr17923-c8256c3. IPA is available here. If you need access to this, you can ask a maintainer to add you.

[jkmassel]

You can test the WordPress changes on this Pull Request by downloading it from AppCenter here with build number: pr17923-c8256c3. IPA is available here. If you need access to this, you can ask a maintainer to add you.

[frosty]

Functionally this is working great for me 👍

However, I after I saw the note about the non-weak reference to the navigation delegate I did some testing with the memory graph debugger and I saw that there's a retain cycle going on – the coordinator and detail view controller were never being released if you tapped in and out of numerous comment notifications. There were also some strong self references used in the various actions used in CommentContentTableViewCell. I've fixed these up on a separate branch – you can see the commits here: https://github.com/wordpress-mobile/WordPress-iOS/compare/feature/17790-arrow_functionality...feature/17790-retain-cycle-fix?expand=1. Hope this helps!

[frosty]

To avoid the duplicate notification when calling this method (e.g. configureWith(notification: notification)) I would suggest naming it configure(with notification: Notification). A call then becomes configure(with: notification).

[ScoutHarris]

What? You don't appreciate ObjC + Swift mashups? 😉 Thank you. Tis been fixed. 😄

[wpmobilebot]

You can test the Jetpack changes on this Pull Request by downloading it from AppCenter here with build number: pr17923-95cb093. IPA is available here. If you need access to this, you can ask a maintainer to add you.

[ScoutHarris]

I've fixed these up on a separate branch

You're a star!! Thank you!! Clearly I didn't see the problem, so yes that most definitely helped.

I've merged your branch, so ready for another go please.

[wpmobilebot]

You can test the WordPress changes on this Pull Request by downloading it from AppCenter here with build number: pr17923-95cb093. IPA is available here. If you need access to this, you can ask a maintainer to add you.

[frosty]

Changes look good! I just remembered something else I forgot to mention previously, however I don't think it should hold this PR up – are we able to do something to stop the view jumping around when you switch comment?

Screen.Recording.2022-02-10.at.10.06.33.mov

[ScoutHarris]

Hey @frosty .

are we able to do something to stop the view jumping around when you switch comment?

In short, @dvdchr had some issues getting the view to resize correctly after the content was loaded, so yea it jumps a bit due to refreshing. I won't be addressing that with this project, but maybe we can look at it in the future.

Thanks!