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

Let delegate control stuff #115

Merged
merged 1 commit into from Jul 7, 2015
Merged

Let delegate control stuff #115

merged 1 commit into from Jul 7, 2015

Conversation

Arthraim
Copy link

Hi @chiunam

Love your work a lot, until I found this design may cause some weird bugs.

What Did I change?

no dismissViewControllerAnimated:completion: call in -dismiss when -assetsPickerControllerDidCancel is implemented

Why?

  1. If you don't implement imagePickerControllerDidCancel:, UIImagePickerController just does't do anything. So, I hope CTAssetsPickerController could stay consistent.
  2. User may want to use other method to dismiss your picker. (Like if I add the view directly to somewhere as a subview, then I want to dismiss it by removing it from super view. That's extremely bad practice though.)
  3. Lost control of completion block of dismissing.

I just deleted the dismissViewControllerAnimated:completion: at first, then I think if user doesn't implement -assetsPickerControllerDidCancel, it should stay what it behaves as you designed, so I put it in an else.

Hope you agree with my thoughts. :D

…ssetsPickerControllerDidCancel is implemented
1and2papa added a commit that referenced this pull request Jul 7, 2015
Allow delegate to control whether to dismiss the picker on cancel
@1and2papa 1and2papa merged commit 1d7bb64 into 1and2papa:master Jul 7, 2015
@1and2papa
Copy link
Owner

@Arthraim Thank you. That's sound reasonable.

@Arthraim
Copy link
Author

Arthraim commented Jul 8, 2015

@chiunam Thank you! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants