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

Integrate PHPickerViewController in Site Icon picker #21370

Merged
merged 4 commits into from
Aug 21, 2023

Conversation

kean
Copy link
Contributor

@kean kean commented Aug 18, 2023

Part of #21190.

  • Replace the WPMediaPicker flow with a context menu
  • Replace the action sheet with the menu (not feature flagged because it's a small visual change)
  • Introduce MediaPickerMenu API for creating the media picker actions. It has a convenient API that ensures the actions are consistent across different parts of the app. The picker uses UIImageViewContoller directly (instead of WPMediaCapturePresenter from WPMediaPicker) – moving forward with the WPMediaPicker removal.
Screenshot 2023-08-21 at 7 50 04 AM

To test:

Important

  • Enable the "Native Photo Picker" feature flag before testing

Choose from Device (Happy Path)”

  • Tap the site icon and select “Choose form Device”
  • Verify that no permission dialog is presented, but you can still access the photos (requires fresh install)
  • Verify that the picker shows only images
  • Select a photo and verify that it shows the Crop screen on top of the picker
  • Crop the photo, tap “Use”, and verify that the icon is uploaded

Choose from Device (Misc)”

  • Verify that if you select a photo and tap “Cancel” on the Crop screen or drag it to dismiss it, you are back to the picker screen where you can change the selection
  • Verify that after changing the selection and going through the Crop flow, the icon is uploaded

Camera

  • Verify that the first time you open the camera, it asks for permissions (requires fresh install)
  • Verify that if you tap “Take Photo”, but the access is restricted, the app shows an alert with an explanation and an “Open Settings” button. Verify the the button opens the Settings app.
  • Take a photo and verify that the crop screen opens and verify that the cropped photo gets uploaded as a site avatar

QuickStart

  • Install the app (fresh install) and login
  • Create a new site
  • Click “Show me Around” when asked
  • Enable Quick Start for New Site (either via debug settings or by creating a new site)
  • Reach the “Choose a unique site icon” step
  • Verify that New Site tour continuing after cancelling the Site Icon menu or going with one of the options

Recordings: menu-cancelled.mp4, icon-updated.mp4.

Removing Icon

  • Set the icon
  • Remove the icon
  • Verify that the “Remove Site Icon” button is no longer visible

Misc

  • Verify that “Create with Emoji” action is not displayed when siteIconCreator is disabled
  • Verify that WebP images can be used (required special handling)

Options

  • let mediaPickerMenu = MediaPickerMenu(presentingViewController: self) modify options and verify video filter and single selection

Flag Disabled

  • Verify that then “Native Photo Picker” is disabled, the UIMenu is still used (for simplicity). I kept only once implementation to make it easier to refactor.

Bonus Points

  • Test all combinations of MediaPickerMenu options (filter and isMultipleSelectionEnabled)
  • Verify that MediaLibraryPickerDataSource and pickers get deallocated after selection or cancellation

Regression Notes

  1. Potential unintended areas of impact: Site Icon selection flow
  2. What I did to test those areas of impact (or what existing automated tests I relied on): manual tests
  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.

UI Changes testing checklist:

  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Aug 18, 2023

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr21370-5eccbcd
Version23.0
Bundle IDcom.jetpack.alpha
Commit5eccbcd
App Center Buildjetpack-installable-builds #5821
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

alert.addDestructiveActionWithTitle(SiteIconAlertStrings.Actions.removeSiteIcon) { [weak self] _ in
NoticesDispatch.unlock()
self?.removeSiteIcon()
func didShowSiteIconMenu() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There doesn't seem to be any good way to track UIMenu dismiss event, so I implemented this "pull-based" check for the site icon flow termination. Not ideal, but works.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Aug 21, 2023

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr21370-5eccbcd
Version23.0
Bundle IDorg.wordpress.alpha
Commit5eccbcd
App Center BuildWPiOS - One-Offs #6781
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

Copy link
Contributor

@momo-ozawa momo-ozawa left a comment

Choose a reason for hiding this comment

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

I tested all the cases, and it works as described! Left some comments


let delegate = ImagePickerDelegate(delegate: delegate)
picker.delegate = delegate
objc_setAssociatedObject(picker, &MediaPickerMenu.strongDelegateKey, delegate, .OBJC_ASSOCIATION_RETAIN_NONATOMIC)
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: are you doing this to ensure the picker / delegate gets deallocated along with the menu?

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 need to retain ImagePickerDelegate that gets created by this method. It gets deallocated along with the image picker.

Base automatically changed from task/phpicker-media-screen to trunk August 21, 2023 19:15
@kean kean force-pushed the task/phpicker-site-icon branch from 0b1ef40 to 1805a9f Compare August 21, 2023 19:42
@kean kean force-pushed the task/phpicker-site-icon branch from 016f000 to 5eccbcd Compare August 21, 2023 19:56
@kean kean enabled auto-merge August 21, 2023 19:56
@kean kean merged commit a6b45fd into trunk Aug 21, 2023
@kean kean deleted the task/phpicker-site-icon branch August 21, 2023 20:27
@kean kean added this to the 23.2 milestone Aug 22, 2023
@kean kean mentioned this pull request Oct 4, 2023
13 tasks
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