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

Reader Comments: Overflow dropdown menu #17601

Merged
merged 3 commits into from
Dec 2, 2021
Merged

Conversation

dvdchr
Copy link
Contributor

@dvdchr dvdchr commented Nov 30, 2021

Refs #17475
Depends on #17600

This PR adds the overflow dropdown menu when the content cell's accessory button is tapped while using an account with moderation roles. Some notes:

  • Please note that showing a context menu (i.e. UIMenu) on tap is only supported in iOS 14 and above. This can be achieved by setting the showsMenuAsPrimaryAction to true and assigning a UIMenu to the menu property of UIButton. Currently, tapping on the ellipsis button does nothing in iOS 13. I'll need to come up with a fallback implementation for iOS 13, and will address this in a separate PR.
  • Most of the tap actions are not yet implemented (except the Share button). This will be addressed in a separate PR.

Simulator Screen Shot - iPhone 12 - 2021-11-30 at 23 55 15

To Test

Ensure that the newCommentThread flag is enabled.

With a moderator account:

  • Go to Reader > any comment thread (from a site that you can moderate).
  • Tap on the ellipsis button on any comment.
  • 🔍 Verify that the context menu is correctly shown and anchored to the accessory view, with all the correct menu items.
  • Tap on the Share menu.
  • 🔍 Verify that a share sheet is presented, containing the comment's URL.

Note: As noted above, in iOS 13.0, tapping on the ellipsis button will do nothing for now.

With a non-moderator account:

  • Go to Reader > any comment thread.
  • Tap on the Share button on any comment.
  • 🔍 Verify that a share sheet is presented, containing the comment's URL.

Regression Notes

  1. Potential unintended areas of impact
    n/a. The feature is hidden behind a flag.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    n/a. The feature is hidden behind a flag.

  3. What automated tests I added (or what prevented me from doing so)
    n/a. The feature is hidden behind a flag.

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.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Nov 30, 2021

You can trigger an installable build for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Nov 30, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@ScoutHarris
Copy link
Contributor

When viewing the new threaded comments, I see a slew of these in the logs:
[UILog] Called -[UIContextMenuInteraction updateVisibleMenuWithBlock:] while no context menu is visible. This won't do anything.

I don't see them with the old threaded comments.

When the menu is shown, I see some of these as well:
[UICollectionViewRecursion] cv == 0x7f7de89e2c00 Disabling recursion trigger logging

I honestly don't know what they mean 😄 , but can they be resolved?

Copy link
Contributor

@ScoutHarris ScoutHarris left a comment

Choose a reason for hiding this comment

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

I left a few comments, but my request for changes is mainly due to the warnings produced by presentMenuForComment.

Base automatically changed from issue/17476-action-bar-handling to develop December 1, 2021 05:41
@dvdchr
Copy link
Contributor Author

dvdchr commented Dec 1, 2021

[UILog] Called -[UIContextMenuInteraction updateVisibleMenuWithBlock:] while no context menu is visible. This won't do anything.

It looks like this is an issue with Xcode 13 targeting iOS 15. I've tried creating a standalone project that shows a menu, and the warning is still there. This doesn't appear when building for iOS 14.5, though!

[UICollectionViewRecursion] cv == 0x7f7de89e2c00 Disabling recursion trigger logging

After searching around, this also seems to be an issue specific to Xcode 13. This only appears when building for iOS 15.

Ready for another round of review, thank you! 🙂

@dvdchr dvdchr requested a review from ScoutHarris December 1, 2021 14:09
@ScoutHarris
Copy link
Contributor

Thanks for looking into those warnings!

Copy link
Contributor

@ScoutHarris ScoutHarris left a comment

Choose a reason for hiding this comment

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

👍 !

:shipit:

@dvdchr dvdchr merged commit e004eb7 into develop Dec 2, 2021
@dvdchr dvdchr deleted the issue/17476-overflow-menu-ui branch December 2, 2021 08: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