Reader Post Details: fetch 2 top level comments #17546
Conversation
|
You can trigger an installable build for these changes by visiting CircleCI here. |
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
| @@ -103,10 +103,13 @@ extern NSUInteger const WPTopLevelHierarchicalCommentsPerPage; | |||
|
|
|||
| // Sync a list of comments sorted by hierarchy, restricted by the specified number of _top level_ comments. | |||
| - (void)syncHierarchicalCommentsForPost:(ReaderPost *)post | |||
| numberTopLevelComments:(NSUInteger)number | |||
| topLevelComments:(NSUInteger)number | |||
There was a problem hiding this comment.
minor nitpick here but I think this needs one more tab for alignment 😄
| @@ -1195,6 +1195,13 @@ - (NSArray *)topLevelCommentsForPage:(NSUInteger)page forPost:(ReaderPost *)post | |||
| return fetchedObjects; | |||
| } | |||
|
|
|||
| - (NSArray *)topLevelComments:(NSUInteger)number forPost:(ReaderPost *)post | |||
| { | |||
| NSArray *comments = [self topLevelCommentsForPage:1 forPost:post]; | |||
There was a problem hiding this comment.
I know this is out of scope and we're only going to pass in 2 here for the comments below post; But since this is a public method, the implementation could be misleading as it only returns comments from page 1.
Should the method also fetch comments from page 2 (or more) in case if there are comments on the next pages and the number is not yet fulfilled?
There was a problem hiding this comment.
Since there are existing methods to get more comments with correct pagination, I purposefully restricted the new ones to page 1 since they are intended to get a minimal number of comments. But you make a good point - it is not clear. So instead of expanding them to allow multiple pages, I added comments noting that they are intentionally restricted to page 1.
| commentService.syncHierarchicalComments(for: post, | ||
| topLevelComments: commentsDisplayed, | ||
| success: { [weak self] _, totalComments in |
There was a problem hiding this comment.
Nothing to be fixed here, just wanted to point out yet another little Xcode 13 bug. The IDE recognizes for as a for-loop syntax instead of a method parameter, and it misaligns the other parameters on the next lines 😅 .
There was a problem hiding this comment.
The IDE recognizes for as a for-loop syntax
Well of course it did. 😆 Thanks!
Ref: #17511
This fetches the post's comments and provides them to the comment table delegate.
syncHierarchicalCommentsfetches the comments and updates Core Data.topLevelCommentsgets the cached comments.updateCommentspasses the comments and total comment count toReaderDetailCommentsTableViewDelegate.The comments are not displayed yet, but the number of rows in the empty table should correspond to the number of comments, plus one for the button row. (i.e. there will be a minimum of 1 row, and a maximum of 3)
To test:
postDetailsCommentsfeature.Regression Notes
Potential unintended areas of impact
N/A. WIP.
What I did to test those areas of impact (or what existing automated tests I relied on)
N/A. WIP.
What automated tests I added (or what prevented me from doing so)
N/A. WIP.
PR submission checklist:
RELEASE-NOTES.txtif necessary.