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

[Pledge Payment Methods] Replace UICollectionView with UIScrollView #772

Conversation

justinswart
Copy link
Contributor

@justinswart justinswart commented Jul 26, 2019

📲 What

This is my suggested fix for the problematic self-sizing UICollectionView that we've been struggling with in the PledgePaymentMethodsViewController. This is not a view that we expect to show a large amount of cells so I don't think we need to be concerned about view recycling. My suggestion is to replace the UICollectionView with a UIScrollView.

🤔 Why

We've been fiddling with the self-sizing UICollectionView issue for 2 days and I don't think it's worth our time anymore. This works very reliably and has zero constraint warnings, even when resizing with dynamic type.

🛠 How

Replace the UICollectionView with a UIScrollView, add the card views to a horizontal UIStackView and enjoy our lives.

👀 See

Size 1 Size 2 Size 3
image image image

let imageViewWidthConstraint = self.imageView.widthAnchor
.constraint(equalToConstant: Layout.ImageView.width)
_ = imageViewWidthConstraint
|> \.priority .~ .defaultHigh
Copy link
Contributor Author

Choose a reason for hiding this comment

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

None of these priority settings were needed for this approach.


self.collectionViewHeightConstraint.constant = self.collectionView.contentSize.height
private func updateScrollViewHeightConstraint() {
self.scrollViewHeightConstraint.constant = self.scrollView.contentSize.height
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So simple.

Copy link
Contributor

@Scollaco Scollaco 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 (so much) for taking a look at this, @justinswart 😄 I left one minor suggestion, but it looks good otherwise.

@@ -228,12 +228,16 @@ public final class RewardCardView: UIView {
}

private func setupConstraints() {
let stateImageViewContainerWidthConstraint = self.stateImageViewContainer.widthAnchor
.constraint(equalToConstant: Styles.grid(5))
stateImageViewContainerWidthConstraint.priority = .defaultHigh
Copy link
Contributor

Choose a reason for hiding this comment

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

To be consistent with how we've set priority, could you use this instead?

let stateImageViewContainerWidthConstraint = self.stateImageViewContainer.widthAnchor
      .constraint(equalToConstant: Styles.grid(5))
|> \.priority .~ .defaultHigh

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 was gonna merge it and make that comment on your PR!

j/k i'll update it.

self.bindViewModel()
self.configureSubviews()
Copy link
Contributor

Choose a reason for hiding this comment

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

🙏 ty!

@@ -228,12 +228,16 @@ public final class RewardCardView: UIView {
}

private func setupConstraints() {
let stateImageViewContainerWidthConstraint = self.stateImageViewContainer.widthAnchor
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change unrelated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It fixes a constraint warning

Copy link
Contributor

@ifbarrera ifbarrera left a comment

Choose a reason for hiding this comment

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

Tested this out on my iPhone X and it seems ok... I'm still a bit wary of how it will perform on older devices for users with > 10 cards, but given some of the data we've pulled on this I'd be open to giving it a try.

@justinswart
Copy link
Contributor Author

Tested this out on my iPhone X and it seems ok... I'm still a bit wary of how it will perform on older devices for users with > 10 cards, but given some of the data we've pulled on this I'd be open to giving it a try.

Sure, I don't think anybody's suggesting this is the ultimate long-term solution, it's more just to get us unstuck on this for now. We can certainly iterate on bringing the collection view back but this allows us to have something that works in the interim.

@justinswart justinswart merged commit bc1ee07 into feature-native-checkout-credit-card-cell Jul 30, 2019
@justinswart justinswart deleted the feature-native-checkout-credit-card-cell-scrollview branch July 30, 2019 00:47
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.

3 participants