wordpress-mobile / MediaPicker-iOS

WPMediaPicker is an iOS controller that allows capture and picking of media assets.
GNU General Public License v2.0
110 stars 37 forks source link

Fix position of empty view to avoid message disappearing when keyboard shows up #399

Closed diogo-lins closed 2 years ago

diogo-lins commented 2 years ago

Fixes #18840, #19514

Description

Issue

https://user-images.githubusercontent.com/20734091/197835979-4dbc38c4-3cc8-4ac3-8c08-69f6f1b60c03.mp4

After changes

Blog Post - With Images Blog Post - Without Images Media - With Images Media - Without Images
Blog-WithImages Blog-WithoutImages Media-WithImages Media-WithoutImages

Gray space before Gray space after

Testing instructions

This can be tested by cloning my fork(and branch) and changing the WordPress-iOS Podfile to use the Pod locally:

pod 'WPMediaPicker', :path => '...'
  1. Run the app
  2. Switch to a site that has no media uploaded
  3. Open Blog Post and add an IMAGE block
  4. Make sure that the custom empty view is displayed correctly
  5. Opens the Keyboard
  6. Make sure that the message No media matching your search is displayed correctly
hassaanelgarem commented 2 years ago

I've also encountered another lower-priority issue.

  1. Open a site with a single media asset
  2. Go to My Site -> Menu -> Media
  3. Select the existing asset and delete it
  4. Try searching for anything
  5. Delete the search query
  6. Notice that the "You don't have any media" screen is hidden behind the keyboard.
diogo-lins commented 2 years ago

The grey space issue is fixed ✅

I found one issue with the no search results string placement. In landscape, the string is partially hidden behind the keyboard on small-screen devices. The below screenshot is from an iPhone SE:

Yeah, you're right, after investigation, this "bug" was created after the fix for this PR, I believe this fix should be made on https://github.com/wordpress-mobile/WordPress-iOS, we need to update the constraint on the NoResultsViewController for this case, this VC that manipulates the text top constraint, should I create an issue and raise the new PR there after this PR be merged and bump the pod version on the WordPress project?

I also mention this issue here: p1666286767668469-slack-C011BKNU1V5

Actual behavior: The below screenshot is from my personal iPhone 13:

unnamed

diogo-lins commented 2 years ago

I've also encountered another lower-priority issue.

  1. Open a site with a single media asset
  2. Go to My Site -> Menu -> Media
  3. Select the existing asset and delete it
  4. Try searching for anything
  5. Delete the search query
  6. Notice that the "You don't have any media" screen is hidden behind the keyboard.

Got it, we just need to update the bottom constraint in this situation too, let me do this. 😄

Resolved here https://github.com/wordpress-mobile/MediaPicker-iOS/pull/399/commits/887de53fbad039df81ba3eb869123248ba6f3d72 😄

centerEmptyVC

diogo-lins commented 2 years ago

Hey @hassaanelgarem 👋🏾, how's it going? I have a question: If a Parent View Controller is using the media picker and implements these two delegates:

- (nullable UIView *)emptyViewForMediaPickerController:(nonnull WPMediaPickerViewController *)picker;
- (nullable UIViewController *)emptyViewControllerForMediaPickerController:(nonnull WPMediaPickerViewController *)picker;

Who has more priority to show? the View Controller or the UIView?

hassaanelgarem commented 2 years ago

Who has more priority to show? the View Controller or the UIView?

I'm not sure, tbh. What is the current behavior? I'd say, whatever the current behavior is, let's keep it. And let's make the priority clear in the documentation of each function.

diogo-lins commented 2 years ago

Who has more priority to show? the View Controller or the UIView?

I'm not sure, tbh. What is the current behavior? I'd say, whatever the current behavior is, let's keep it. And let's make the priority clear in the documentation of each function.

Following the current behavior, the empty VC has more priority.

hassaanelgarem commented 2 years ago

The single media asset issue is resolved 🚀

we need to update the constraint on the NoResultsViewController for this case, this VC that manipulates the text top constraint, should I create an issue and raise the new PR there after this PR be merged and bump the pod version on the WordPress project?

There's no need to block this fix until #399 is merged, we should actually aim to test both concurrently.

Please create a PR on WPiOS with this fix before we merge this PR. For now, the WPiOS PR should point to the WPMediaPicker living on the branch issue/18840-19514-emptyview-height-v2. Once #399 is merged and the new version is released, we can update the WPiOS PR to point to the released version.

diogo-lins commented 2 years ago

The single media asset issue is resolved 🚀

we need to update the constraint on the NoResultsViewController for this case, this VC that manipulates the text top constraint, should I create an issue and raise the new PR there after this PR be merged and bump the pod version on the WordPress project?

There's no need to block this fix until #399 is merged, we should actually aim to test both concurrently.

Please create a PR on WPiOS with this fix before we merge this PR. For now, the WPiOS PR should point to the WPMediaPicker living on the branch issue/18840-19514-emptyview-height-v2. Once #399 is merged and the new version is released, we can update the WPiOS PR to point to the released version.

Got it! Let me do this 🚀