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

Notifications > Comments: show Notification detail from previous/next buttons #17959

Merged
merged 2 commits into from
Feb 16, 2022

Conversation

ScoutHarris
Copy link
Contributor

Ref: #17790

When viewing Comment notification details from the All filter, show Notification details view if the previous/next notification is not a Comment.

To test:

  • Enable notificationCommentDetails.
  • Go to Notifications > All.
  • Select a Comment notification that has a non-Comment notification before/after it.
  • On the comment details, select previous/next on the nav bar.
  • Verify the Notification details is displayed.
    • Note: sometimes a Post notification doesn't render any content. This is an unrelated preexisting issue.

Regression Notes

  1. Potential unintended areas of impact
    N/A. Feature is incomplete and 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.

@ScoutHarris
Copy link
Contributor Author

I noticed the Comment header doesn't work as expected (sometimes nothing happens when tapped, doesn't change with Comment). I'll look at it separately.

Copy link
Contributor

@dvdchr dvdchr left a comment

Choose a reason for hiding this comment

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

Tested and it works. LGTM! :shipit:


viewController.dismiss(animated: true, completion: {
notificationDetailsViewController.navigationItem.largeTitleDisplayMode = .never
navigationController?.popViewController(animated: false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just realized that with the viewController.dismiss and this line, this looks like the view controller is popped twice. But then I tested, and it just works 🌈 😄.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, it feels wrong. 😄 But yea that's the only way it works, both calls are necessary. 🤷

@ScoutHarris
Copy link
Contributor Author

Thanks @dvdchr !

@ScoutHarris ScoutHarris merged commit cb0c17f into trunk Feb 16, 2022
@ScoutHarris ScoutHarris deleted the feature/17790-change_notif_type_2 branch February 16, 2022 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants