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

[NT-1240] PledgeViewController layout bugfix #1187

Merged
merged 9 commits into from
May 12, 2020
Merged

Conversation

justinswart
Copy link
Contributor

@justinswart justinswart commented May 7, 2020

πŸ“² What

Fixes an inconsistent layout bug in PledgeViewController.

πŸ€” Why

We discovered an issue that would cause PledgeDisclaimerViewController and PledgeDescriptionViewControllers to be arbitrarily taller than necessary.

πŸ›  How

This was a difficult one to track down but in the end @ifbarrera and I were able to prevent it from happening by changing the PledgeDisclaimerViewController and PledgeDescriptionViewController to be regular UIViews instead of nested UIViewControllers. It's still not entirely clear to us why this change fixes the problem but it'll do for now.

In trying to track this down I was also able to simplify the layout and prevent an ambiguous content size in the rootScrollView.

πŸ‘€ See

Before πŸ› After πŸ¦‹
image image

βœ… Acceptance criteria

Navigate to the pledge view.

  • All views should have the correct height.
  • The heights should be correct for rewards that have shipping.
  • The heights should be correct for rewards that don't have shipping.
  • Dragging/interacting with the view while payment methods and shipping loads should not affect the layout.

@@ -144,6 +144,10 @@ final class PledgeAmountViewController: UIViewController {
}
}

override func didMove(toParent parent: UIViewController?) {
self.verticalSpacer.isHidden = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This spacer is not needed when this view is contained and was causing the scrollview to have an ambiguous content size.

override func viewDidLayoutSubviews() {
super.viewDidLayoutSubviews()

self.rootScrollView.contentInset = UIEdgeInsets(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusting content insets instead of pinning the scrollview to the top of the button container.

Copy link
Contributor

@jgsamudio jgsamudio left a comment

Choose a reason for hiding this comment

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

@justinswart I was only able to reproduce this on a 11 pro max, however when you tap "new card payment" and then dismiss the view, the "kickstarter is not a store" view is stuck below the pledge buttons.

@justinswart
Copy link
Contributor Author

I was only able to reproduce this on a 11 pro max, however when you tap "new card payment" and then dismiss the view, the "kickstarter is not a store" view is stuck below the pledge buttons.

@jgsamudio thanks for testing. I wasn't able to reproduce - could you perhaps attach a screenshot?

@justinswart justinswart merged commit 607822d into master May 12, 2020
@justinswart justinswart deleted the NT-1240-layout-bugs branch May 12, 2020 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants