nytimes / NYTPhotoViewer

A modern photo viewing experience for iOS.
http://open.blogs.nytimes.com/2015/03/27/a-new-view-for-nytimes-photos/
Other
2.86k stars 378 forks source link

Dismiss function public #72

Closed phillebaba closed 8 years ago

phillebaba commented 9 years ago

It would be great if we could make the dismiss function public to that the viewcontroller can be removed through custom logic. An option would be to access the done button method.

- (void)doneButtonTapped:(id)sender {
    self.transitionController.forcesNonInteractiveDismissal = YES;
    [self setOverlayViewHidden:YES animated:NO];
    [self dismissAnimated:YES];
}
livings124 commented 8 years ago

Having dismiss function public, with an animation completion block would be very useful. I've hacked together a solution where we pull the dismiss method from the dismiss button, but that's clearly not the best solution.

jaybowang commented 8 years ago

For uses who are not familiar with the swipe-up/down-to-dismiss-gesture it's a little confusing. I watched two of my friends who haven't used this before and they both didn't know what to do for seconds. It would be better to add a close bar button item at the upper-left corner. Just like a close button in a normal modal view controller which people know for a long time.

I agree with @phillebaba and @livings124 .Want dismissAnimated: too.

Currently I'm using swift code photoViewer.performSelector(Selector("dismissAnimated:"), withObject: true) but no animation.

cdzombak commented 8 years ago

…It would be better to add a close bar button item at the upper-left corner.…

There is supposed to be one but it's broken on develop right now; see issue https://github.com/NYTimes/NYTPhotoViewer/issues/102 .

I agree with @phillebaba and @livings124 .Want dismissAnimated: too.

Re: this proposal and the PR https://github.com/NYTimes/NYTPhotoViewer/pull/96 , I've been holding off exposing dismissAnimated: because we believe the correct approach is to make the standard UIViewController method -dismissViewControllerAnimated:completion: work properly. Clearly it doesn't right now, but we believe it should; introducing a view controller which requires calling a custom method for dismissal feels wrong & breaks inheritance.

I haven't had time to dig into this to see what's required to make this work, but I would prefer making -dismissViewControllerAnimated:completion: work properly before we consider exposing the existing internal -dismissAnimated: method.

cdzombak commented 8 years ago

Please see #109 for further discussion.