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

Recommend App: Add presenter to encapsulate display logic #17006

Merged
merged 17 commits into from
Aug 19, 2021

Conversation

dvdchr
Copy link
Contributor

@dvdchr dvdchr commented Aug 11, 2021

Refs #16995
Closes #17003
Based on #17005

Summary

This change introduces a presenter object that encapsulates content fetching and presentation logic for the share app flow, along with a subclass of UIActivityItemSource to help format the text contents depending on the activity type. As described in #17003, I think it's worth it to encapsulate this since it's possible to have multiple entry points in the future.

Some notes for this change:

  • The presenter exposes a delegate that gets called on every loading state changes. This is useful for the caller to update their display to show e.g. a loading indicator while the content is being fetched.
  • Other than formatting needs, a UIActivityItemSource subclass is necessary to provide additional information – specifically a subject string provided through activityViewController:subjectForActivityType. This information is used when sharing to mail. I'll share some screenshots below on how it looks when it's shared in various apps.
  • Lastly, this adds a new feature flag for the recommend button.

Screenshots

Share Destination Screenshot
Messages share_app_messages
Mail share_app_mail
Twitter share_app_twitter
WordPress (Save as Draft) – Note: Not sure why the text contents are not carried, but I suspect this needs to be fixed from the WP side. share_app_wp_draft
Copy (Pasteboard) share_app_copyToPasteboard
Chrome share_app_chrome
Slack – Not sure why the text is not present, but this is usually due to the destination's Share Extension not picking up multiple activity items. share_app_slack
WhatsApp share_app_whatsapp

To Test

Manually editing the source + compiling to device

Since there are no visual components hooked up to this component, we'll have to wire this up manually in Xcode AND compile it to a device 😅 .

Here's how I modified the code to test the share functionality:

  • In the MeViewController, add a private instance variable like so:
    private let sharePresenter = ShareAppContentPresenter(account: nil)
  • Find the pushHelp() method, comment out the code in the return block, and change it to this:
    return { [unowned self] row in
        defer { self.tableView.deselectSelectedRowWithAnimation(true) }
        guard let indexPath = self.tableView.indexPathForSelectedRow,
              let selectedCell = self.tableView.cellForRow(at: selectedIndexPath) else {
            return
        }
        sharePresenter.present(for: .wordpress, in: self, sourceView: selectedCell)
    }
  • Since there's no loading indicator yet, just wait for one or two seconds and the share sheet should appear shortly after tapping the help button on the Me screen.
  • Lastly, try logging in to a self-hosted site (without WP account) and make sure that you can still prompt the share sheet.

Unit tests

Make sure that the tests are 🟢 !

Regression Notes

  1. Potential unintended areas of impact
    None.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    This component is not used yet.

  3. What automated tests I added (or what prevented me from doing so)
    Added some unit tests for the presenter.

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.

@dvdchr dvdchr added this to the 18.1 milestone Aug 11, 2021
@dvdchr dvdchr requested review from aerych and ScoutHarris August 11, 2021 19:01
@dvdchr dvdchr self-assigned this Aug 11, 2021
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Aug 11, 2021

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

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Aug 11, 2021

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

@dvdchr dvdchr marked this pull request as ready for review August 12, 2021 18:57
@dvdchr dvdchr linked an issue Aug 12, 2021 that may be closed by this pull request
5 tasks
Copy link
Member

@aerych aerych left a comment

Choose a reason for hiding this comment

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

Howdy @dvdchr 👋
I tested on my iPhone X and everything worked great. Thanks for showing all the examples in the PR description. It was nice to have something to compare my results to. :)
Code looks good to me. I had just one suggestion about the subject text.
Otherwise :shipit: :)

Copy link
Member

@aerych aerych left a comment

Choose a reason for hiding this comment

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

Spotted an issue so I've added another note and marked this request changes since we need to address an exception. Sorry I missed it in the first round!

@dvdchr dvdchr requested a review from aerych August 18, 2021 07:26
Base automatically changed from issue/17002-button-with-icon-cell to develop August 18, 2021 08:13
Copy link
Member

@aerych aerych left a comment

Choose a reason for hiding this comment

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

Heya @dvdchr 👋
The exception is resolved. Thanks for the changes! Marking approved so as to not block the rest of the work.

However, I think we might want to see if we can improve the popover presentation on iPad. The solution I suggested was something I hadn't tested, and now that I see it in action I think we might need to make an adjustment.

Depending on which way I orient the iPad, the popover appears away from where I tapped. The size also varies depending on orientation so sometimes there are more visible options than others. I'm wondering if sharing the frame of the cell (or maybe just the cell as the view) that was tapped rather than the controller view would help improve the experience. We'd also need to make the change before the code freeze. What do you think?
Simulator Screen Shot - iPad Pro (9 7-inch) - 2021-08-18 at 11 50 02
Simulator Screen Shot - iPad Pro (9 7-inch) - 2021-08-18 at 11 50 05

@dvdchr
Copy link
Contributor Author

dvdchr commented Aug 19, 2021

Hey @aerych. I wasn't very aware of the design guidelines for iPad before, so thank you for the help and tips on this. Really appreciate it! 🙂

Depending on which way I orient the iPad, the popover appears away from where I tapped. The size also varies depending on orientation so sometimes there are more visible options than others. I'm wondering if sharing the frame of the cell (or maybe just the cell as the view) that was tapped rather than the controller view would help improve the experience.

Noted. I just realized that since the popover is anchored to the view controller's view, there's not enough space on the side and the system puts the popover on the top side, which makes the content a bit cramped.

Addressed in 6a07b6d, I've confirmed that passing the cell as the sourceView correctly anchors the popover to the tapped cell, and indeed the share sheet is displayed better. Here's a preview:

Portrait Landscape
ipad_portrait ipad_landscape

We'd also need to make the change before the code freeze. What do you think?

Since the code review for #17007 and #17020 is blocked by this one, I'll apply the changes on this PR. Then, I'll merge up the changes to both #17007 and #17020, so you can proceed with reviewing both of them.

I'll aim to merge everything to develop tomorrow (provided that you approved them all, of course 😄 ). If there's any new major issue on the PRs, I will address it on a separate PR (since the feature flag is still false).


P.S.: I've updated the code sample in the description to fill the sourceView parameter if you decided to give this another look! 🙂

@peril-wordpress-mobile
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@aerych
Copy link
Member

aerych commented Aug 19, 2021

Tested with the changes and they look great! Thanks @dvdchr! :shipit: again!

@dvdchr
Copy link
Contributor Author

dvdchr commented Aug 19, 2021

Thanks @aerych !

@dvdchr dvdchr merged commit 33b3c9b into develop Aug 19, 2021
@dvdchr dvdchr deleted the issue/17003-share-app-content-presenter branch August 19, 2021 15:42
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.

Add component to handle content fetching and presentation logic
3 participants