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

Gallery overlay view does not cover whole screen when presented in a modal view controller #85

Merged
merged 3 commits into from
Nov 15, 2016
Merged

Gallery overlay view does not cover whole screen when presented in a modal view controller #85

merged 3 commits into from
Nov 15, 2016

Conversation

ivopintodasilva
Copy link
Contributor

Overlay view added to the back of the gallery instead of the original view controller.

Copy link
Collaborator

@zfoltin zfoltin left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @ivopintodasilva 👍
Some comments there, once these are resolved I'm happy to merge it

@@ -149,16 +149,17 @@ open class GalleryViewController: UIPageViewController, ItemControllerDelegate {

NotificationCenter.default.removeObserver(self)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary whitespace

overlayView.center = CGPoint(x: (UIScreen.main.bounds.width / 2), y: (UIScreen.main.bounds.height / 2))

/// Add the overlay to the back of the gallery view controller instead of the original one
/// This fixes the problem of not opening the gallery in fullScreen when it's originated on a Modal controller
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's get rid of these comments, they are meaningless without knowing how it worked before

P.S. we generally comment using // not ///

fileprivate func configureOverlayView() {

/// Make the overlay view occupy the whole screen
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that's what the next line does. The intention here is to give the overlay enough padding so it still covers the screen when rotated.
Let's get rid of this comment, people will understand what the next line does without it

@ivopintodasilva
Copy link
Contributor Author

Comments resolved, @zfoltin

However, at first, I didn't understand why the overlayView size wasn't simply "UIScreen.main.bounds.size".

Maybe because in my use case I only use the gallery in portrait mode, I didn't account for the rotation. Maybe a comment on that would turn it clearer for future contributors.
Just a suggestion, keep up the amazing work 💪

@zfoltin zfoltin merged commit 42fa11d into Krisiacik:master Nov 15, 2016
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.

2 participants