Skip to content

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

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