Notifications > Comment Details: show new details view for cached comment #17850
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…omment details view for a comment notification.
37 tasks
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
frosty
approved these changes
Feb 1, 2022
Comment on lines
+729
to
+741
| let notificationCommentDetailCoordinator = NotificationCommentDetailCoordinator(notification: note) | ||
|
|
||
| // For now, NotificationCommentDetailCoordinator only loads the Comment if it is cached. | ||
| // If the comment is not cached, fall back to showing the old comment view. | ||
| // This is temporary until NotificationCommentDetailCoordinator can fetch the comment from the endpoint. | ||
|
|
||
| if let commentDetailViewController = notificationCommentDetailCoordinator.viewController { | ||
| commentDetailViewController.navigationItem.largeTitleDisplayMode = .never | ||
| self.showDetailViewController(commentDetailViewController, sender: nil) | ||
| } else { | ||
| // TODO: remove when NotificationCommentDetailCoordinator updated to fetch comment. | ||
| self.performSegue(withIdentifier: NotificationDetailsViewController.classNameWithoutNamespaces(), sender: note) | ||
| } |
There was a problem hiding this comment.
Nitpick, but as this method is already pretty long, could we extract these into a presentCommentDetail(for: note) method or something?
There was a problem hiding this comment.
Sure. I'll do it in the next PR.
|
Thanks @frosty ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ref: #17790
This is the first step in showing the new Comment Details view for a Comment Notification. Specifically, when a comment notification is selected, if the comment is already cached, it will be displayed with the new view.
Technical details: because
CommentDetailViewControllerrequires aCommentobject, theCommentmust first be loaded with information from theNotification.NotificationCommentDetailCoordinatorhas been added to facilitate comment loading, as well as other Notification specific functionality that will be added later.Of course since this is the first step in showing this view, there are a number of outstanding tasks, which are listed in the referenced issue.
To test:
notificationCommentDetailsfeature.Regression Notes
Potential unintended areas of impact
N/A. Feature is incomplete and disabled.
What I did to test those areas of impact (or what existing automated tests I relied on)
N/A. Feature is incomplete and disabled.
What automated tests I added (or what prevented me from doing so)
N/A. Feature is incomplete and disabled.
PR submission checklist:
RELEASE-NOTES.txtif necessary.