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

iPhone landscape support #764

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

igor-gini
Copy link
Contributor

No description provided.

@zladzeyka zladzeyka assigned zladzeyka and igor-gini and unassigned zladzeyka Jan 17, 2025
@igor-gini igor-gini changed the title [WIP] PP-943: support for camera, error screen, no results screen, album picker screen, photo selector screen; fix camera crashing for ipad iPhone landscape support Jan 21, 2025
@zladzeyka zladzeyka self-requested a review January 27, 2025 16:27
@igor-gini igor-gini force-pushed the iphone-landscape-orientation-support branch from 3032c97 to 1024084 Compare January 31, 2025 14:03
@igor-gini igor-gini force-pushed the iphone-landscape-orientation-support branch from 2a11e17 to f482cb1 Compare February 20, 2025 08:28
Copy link
Collaborator

@zladzeyka zladzeyka left a comment

Choose a reason for hiding this comment

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

Thank you @igor-gini
I left some comments here. I didn't check the bottom navigation yet, I'll check it as you're done with suggestions

@@ -28,6 +28,7 @@ extension UITableViewCell {

public struct GiniMargins {
public static let margin: CGFloat = 16
public static let marginHorizontal: CGFloat = 56
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public static let marginHorizontal: CGFloat = 56
public static let horizontalMargin: CGFloat = 56

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMG_4655

we need to consider Dynamic Island here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically, you just need to anchor elements horizontally not to the view itself, but to its safeAreaLayoutGuide, and then use leadingAnchor/trailingAnchor from it as needed. That way, it will be attached to the safe area.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -71,7 +71,6 @@ class OnboardingViewController: UIViewController {
NSLayoutConstraint.activate([
navigationBar.topAnchor.constraint(equalTo: pageControl.bottomAnchor, constant: 46),
Copy link
Collaborator

Choose a reason for hiding this comment

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

@igor-gini please wrap it in Constants

pageControl.bottomAnchor.constraint(equalTo: contentView.bottomAnchor, constant: -Constants.padding * 2),
pageControl.topAnchor.constraint(equalTo: collectionView.bottomAnchor, constant: Constants.padding * 2),
pageControl.leadingAnchor.constraint(equalTo: contentView.leadingAnchor),
pageControl.trailingAnchor.constraint(equalTo: contentView.trailingAnchor, constant: -275)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@igor-gini please wrap it in Constant

@@ -272,6 +278,18 @@ public final class ReviewViewController: UIViewController {
buttonContainer.leadingAnchor.constraint(greaterThanOrEqualTo: view.leadingAnchor)
]

private lazy var buttonContainerHorizontalConstraints: [NSLayoutConstraint] = [
buttonContainer.trailingAnchor.constraint(equalTo: view.trailingAnchor, constant: -85),
Copy link
Collaborator

Choose a reason for hiding this comment

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

@igor-gini please wrap it in Constants

bottomNavigationBar?.alpha = isLandscape ? 0 : 1
bottomNavigationBar?.isUserInteractionEnabled = !isLandscape

trailingConstraints.forEach { $0.constant = isLandscape ? -275 : 0 }
Copy link
Collaborator

Choose a reason for hiding this comment

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

@igor-gini please wrap it in Constants

Copy link
Collaborator

Choose a reason for hiding this comment

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

@igor-gini Could you please wrap the magic numbers in the Constants?
Our file is getting too big, can we extract something in extension and move it to a new file?
IMG_9893
Currently if I add 3 images scrollview is not positioned correctly could you please adjust it as we have here https://www.figma.com/design/ZYpdKfpaHOpV7RV1TTWdEH/Gini-Photo-Payment%3A-iOS-%26-Android?node-id=9243-41741&t=RYL3FUZBCL6TItSy-1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn’t quite understand the ScrollView positioning here. In the code, I see that it highlights the last captured photo after shooting, but should it be the second-to-last one? Or how is it supposed to work?

@zladzeyka zladzeyka self-requested a review March 4, 2025 17:12
Copy link

sonarqubecloud bot commented Mar 7, 2025

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