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

Use safe area guides for text label and shutter button #69

Merged
merged 3 commits into from
Feb 14, 2018

Conversation

gerriet
Copy link
Contributor

@gerriet gerriet commented Feb 13, 2018

To improve the layout especially on iPhoneX, the safe area guides are used for

  • text label - might be partially occluded by the notch
  • shutter button - might be too close to the border

Copy link
Owner

@dokun1 dokun1 left a comment

Choose a reason for hiding this comment

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

this looks great, thank you so much @gerriet !!

As a favor, since I don't have an iPhone X, could you post screenshots of what the new layout looks like on one? I'll take a peek on my own device (iPhone 7+) today.

@gerriet
Copy link
Contributor Author

gerriet commented Feb 13, 2018

@dokun1, happy that you like the small contribution. I have attached two screenshots in portrait and landscape but also the same images as photos from my iPad looking at my iPhone X (as the notch and edges are removed in screenshots). Hope this helps. I might to also have a look at the camera switch button that can be a little too close to the rounded corner.
PS: Looking forward to see what I can do with Lumina, my first impression is that it is a great resource to get started with applying image recognition models to camera input.

Screenshot portrait:
screenshot_portrait
Screenshot landscape:
screenshot_landscape
iPhone X portrait:
phone_portrait
iPhone X landscape:
phone_landscape

@dokun1
Copy link
Owner

dokun1 commented Feb 13, 2018

@gerriet Before I merge, how would you feel about adding some logic to have the text area span the entire width if it gets moved down for the notch? It looks like it would fit...

Let me know your thoughts!

if #available(iOS 11, *) {
minY = self.safeAreaLayoutGuide.layoutFrame.minY
}
minY += 5.0
Copy link
Owner

Choose a reason for hiding this comment

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

Right here, we could perhaps put a check to see how much minY has changed, and update self.frame.size based on this value. I would love to do this, but not having an iPhone X means I dont know what value to expect ¯_(ツ)_/¯

@gerriet
Copy link
Contributor Author

gerriet commented Feb 13, 2018

The label will be moved down 39 points. Although full width for the label would be nice on this slim and high device, I am not sure about it, because I think the camera switch button will have to be moved out of the corner slightly, which will put text and button very close to each other. Will try it out tomorrow.

Moved a little layout logic to the proper place
@gerriet
Copy link
Contributor Author

gerriet commented Feb 14, 2018

I implemented the change you suggested and think that the distance right now is large enough and also that it really helps to have more width for the label. See also attached screenshot (iPhone X in portrait is the only case where this change is effective). I also moved some of the stuff I did from LuminaTextPromptView to LuminaViewController::updateButtonFrames(), where I think it fits better.
img_0256

@dokun1
Copy link
Owner

dokun1 commented Feb 14, 2018

LGTM - anything else to change before merging?

@gerriet
Copy link
Contributor Author

gerriet commented Feb 14, 2018

I'd say "let's go"

@dokun1 dokun1 merged commit 85d1651 into dokun1:master Feb 14, 2018
@dokun1
Copy link
Owner

dokun1 commented Feb 14, 2018

Thank you @gerriet for contributing 👍

@dokun1 dokun1 mentioned this pull request Mar 5, 2018
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