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 > Comment Details: show previous and next arrows in nav bar #17897

Merged
merged 5 commits into from
Feb 8, 2022

Conversation

ScoutHarris
Copy link
Contributor

@ScoutHarris ScoutHarris commented Feb 4, 2022

Ref: #17790

On the new Notification Comment details view, previous and next buttons are now displayed in the nav bar.

They're not hooked up yet, so:

  • For now tapping them simply logs a message (just to show they work).
  • They are not disabled if there is no previous/next notification.

To test:

  • Enable notificationCommentDetails.
  • Go to Notifications > Comments > notification details.
    • The Edit button is always displayed (all devices, all orientations).
    • On an iPad, the arrow buttons are never displayed.
    • On a regular size iPhone, the arrow buttons are always displayed.
    • On an X size iPhone:
      • In portrait, the arrow buttons are displayed.
      • In landscape, the arrow buttons are not displayed.
  • Go to My Site > Comments > comment details.
    • Verify only the Edit button is displayed.
Arrows No Arrows
arrows_shown arrows_not_shown

Regression Notes

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

Copy link
Contributor

@frosty frosty left a comment

Choose a reason for hiding this comment

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

This works for me, however I'm seeing a weird animation in the buttons when the view appears:

Screen.Recording.2022-02-07.at.11.03.48.mov

I'm also not sure if the button names and labels should be reversed. The up arrow will presumably show the notification above in the list, which would be the next notification. The down arrow would show the notification below, which chronologically would be the previous notification.

navigationItem.setRightBarButtonItems(barButtonItems, animated: false)
}

func navigationButtons() -> UIBarButtonItem {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, but I'd either rename this to makeNavigationButtons or move it to a lazy var named something like navigationButtonsStackView.

@ScoutHarris
Copy link
Contributor Author

The up arrow will presumably show the notification above in the list, which would be the next notification...

I was thinking up arrow is previous in the list (hence previousButton), but I can see it also means next notification. I suppose it depends on how it's interpreted. Anyway, it doesn't matter to me, I've changed it.

a weird animation in the buttons when the view appears

Eagle eyes. 😄 I removed a redundant call to configureNavigationBar which seems to have resolve that.

Ready for another go please!

@ScoutHarris ScoutHarris requested a review from frosty February 7, 2022 21:04
@jkmassel
Copy link
Contributor

jkmassel commented Feb 7, 2022

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

@jkmassel
Copy link
Contributor

jkmassel commented Feb 7, 2022

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

Base automatically changed from feature/17790-show_fetched_comment to trunk February 7, 2022 21:56
Copy link
Contributor

@frosty frosty left a comment

Choose a reason for hiding this comment

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

:shipit:

@ScoutHarris
Copy link
Contributor Author

Thanks @frosty!

@ScoutHarris ScoutHarris merged commit 3df4ab5 into trunk Feb 8, 2022
@ScoutHarris ScoutHarris deleted the feature/17790-nav_bar_arrows branch February 8, 2022 19:04
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.

3 participants