Skip to content
This repository was archived by the owner on Jun 19, 2025. It is now read-only.

Conversation

@ScoutHarris
Copy link
Contributor

@ScoutHarris ScoutHarris commented Jul 31, 2018

Ref #wordpress-mobile/WordPress-iOS#9504

This adds the ability to use a ViewController for the emptyView state, instead of just a UIView. Specifically because we are transitioning from WPNoResultsView to NoResultsViewController in WordPress-iOS .

To test:

Hey @etoledom - it looks like you might be familiar with the emptyView logic. Would you mind taking a look at this? Thanks!!

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

Hey @ScoutHarris - this is a great addition, thank you!

I have a question that I wrote as a code comment about having two different defaults.

Apart of that, I noticed a small issue with the position of the view when it refreshes. It reappears under the keyboard.
It also seems to disappear when the search bar is cleared.

Maybe we can call [self centerEmptyView]; as soon as we add the emptyViewController?

no_results

And one small detail: When the search is cancelled, it looks like the empty controller disappear and then appears again in the lower position. Is it possible to make it slide down the same way it slide up when the keyboard appears?

This is not life or death but a "nice to have". 😁

The default empty view controller. When `emptyViewControllerForMediaPickerController:` is not implemented,
use this property to style the message.
*/
@property (nonatomic, strong, readonly, nonnull) UIViewController *defaultEmptyViewController;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to have both a defaultEmptyView and a defaultEmptyViewController?
Probably we should just have one default that will be displayed if non of the related delegates method are implemented. Does it make sense?

Not sure which would be the best option, but I'd vote for the simplest of them.

Copy link
Contributor Author

@ScoutHarris ScoutHarris Aug 1, 2018

Choose a reason for hiding this comment

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

Hey @etoledom . Good point...

So, WPNavigationMediaPickerViewController needs access to defaultEmptyViewController, so I added a method definition. I also removed defaultEmptyViewController as a public property, and changed it to use defaultEmptyView.

…ltEmptyViewController` use `defaultEmptyView` instead of creating its own. Add `defaultEmptyViewController` accessor since `WPNavigationMediaPickerViewController` needs it.
…e issue where it was appearing under the keyboard when it refreshed.
@ScoutHarris
Copy link
Contributor Author

ScoutHarris commented Aug 1, 2018

Hey @etoledom . Thanks for the timely review and helpful feedback!

Some comments in response -

It reappears under the keyboard.

Adding [self centerEmptyView]; did in fact fix it.

When the search is cancelled, it looks like the empty controller disappear and then appears again in the lower position. Is it possible to make it slide down the same way it slide up when the keyboard appears?

It was not being re-added, it was just re-centering immediately. So I added an animateWithDuration block to slow it down a bit.

It also seems to disappear when the search bar is cleared.

I'm not seeing this. If you still do, can you provide more information? (device, iOS version, anything else helpful)

Otherwise, it's ready for another review!

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

@ScoutHarris - nice fixes! 🎉

  • The animation when dismissing the keyboard is perfect!
  • The position of the empty view controller content also got fixed 🎉
  • The empty controller doesn't disappear any more :]

I saw a new small issue:

It happens when I get the no-results message and then I clear the search text. The empty view keeps showing the no-results message (with the last string I used to search), but it should show the initial empty message (that one with the Photos provided by pexel). Here is a gif showing it:

media_picker_empty_controller


About the defaultEmptyViewController, I see what you mean by WPNavigationMediaPickerViewController needs access to it.

What if WPNavigationMediaPickerViewController returns nil if the method is not implemented bit the delegate, and WPMediaPickerViewController handles nil in the same way than respondsToSelector == NO?

I was experimenting a bit with that idea but I got the feeling that handling nil might make everything more complicated. You can decide if it's worth it.

My concern is that by having both defaults seems like one of them will never be used, so we should default just to one.

@ScoutHarris
Copy link
Contributor Author

Hey @etoledom !

More responses:

The empty view keeps showing the no-results message

Well that was weird. I could only get it to happen on a first run on a device. Successive runs worked fine. Good catch! It was actually a problem on the WP-iOS side, and has been fixed (wordpress-mobile/WordPress-iOS@efb1be0).

Regarding the defaultEmptyViewController:

What if WPNavigationMediaPickerViewController returns nil if the method is not implemented bit the delegate, and WPMediaPickerViewController handles nil in the same way than respondsToSelector == NO?

I was actually trying not to mix emptyView and emptyViewController so 1) the logic would be clear and independent; 2) you get what you expect. Meaning if you're using a view, a view is displayed; if you're using a view controller, a view controller is displayed.

My concern is that by having both defaults seems like one of them will never be used, so we should default just to one.

I do see your point. But "one of them will never be used" isn't exactly accurate - it will be one or the other.

However, looking into the future, after the NoResultsView replacement is complete in WordPress-iOS, emptyView will no longer be needed over there. I was considering removing it and defaultEmptyView. I just don't know if that's safe to do.

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

It was actually a problem on the WP-iOS side, and has been fixed

That issue is fixed now, great work!

  • The animation issue when dismissing the keyboard seems to have reappeared, but as we talked internally let's merge this and fix the animation separately.

@ScoutHarris ScoutHarris merged commit 9fffaa4 into develop Aug 3, 2018
@ScoutHarris ScoutHarris deleted the issue/9504-no_results_vc_support branch August 3, 2018 17:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants