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

Remove Rewards Collection View Custom Centering Behavior #793

Merged
merged 13 commits into from
Aug 14, 2019

Conversation

ifbarrera
Copy link
Contributor

@ifbarrera ifbarrera commented Aug 12, 2019

📲 What

This PR removes our custom "hidden scroll view" implementation of the cell-centering behavior and replaces it with UIScrollViews out-of-the-box free-scrolling behavior.

🤔 Why

To simplify rewards browsing for the first release of this redesign.

🛠 How

  • removes the hidden UIScrollView
  • removes UIScrollViewDelegate implementation
  • updates some constraints

👀 See

G0DFNMl9Wy

✅ Acceptance criteria

  • With the "Native Checkout" feature branch turned ON navigate to any project and tap "Back this project". You should be able to freely scroll through rewards, and the horizontal scroll indicator should be visible.
  • With the "Native Checkout" feature branch turned ON navigate to any project and tap "Back this project". Turn the phone to landscape mode. You should be able to freely scroll through rewards, and the horizontal scroll indicator should be visible.

@@ -161,8 +161,7 @@ final class PledgeCTAContainerView: UIView {
self.activityIndicator.centerXAnchor.constraint(equalTo: self.layoutMarginsGuide.centerXAnchor),
self.activityIndicator.centerYAnchor.constraint(equalTo: self.layoutMarginsGuide.centerYAnchor),
self.pledgeCTAButton.heightAnchor.constraint(greaterThanOrEqualToConstant: Layout.Button.minHeight),
self.pledgeCTAButton.widthAnchor.constraint(greaterThanOrEqualToConstant: Layout.Button.minWidth),
self.rootStackView.topAnchor.constraint(equalTo: self.layoutMarginsGuide.topAnchor)
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 constraint was causing warnings because it conflicts with the constraints applied here:

https://github.com/kickstarter/ios-oss/blob/master/Kickstarter-iOS/Views/PledgeCTAContainerView.swift#L145

@ifbarrera ifbarrera marked this pull request as ready for review August 12, 2019 17:39
Copy link
Contributor

@dusi dusi left a comment

Choose a reason for hiding this comment

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

Looks good...we'll be able to provide functionality without weird hacks :)

I've noticed that we no longer use ksr_insertSubviewInParent, we should probably remove it (for one it's super trivial to add it back and we'll clean up the codebase).

Kickstarter-iOS/Views/Cells/RewardCell.swift Show resolved Hide resolved
Kickstarter-iOS/Views/Cells/RewardCell.swift Outdated Show resolved Hide resolved
super.viewWillTransition(to: size, with: coordinator)

self.flowLayout?.invalidateLayout()
if itemSize != layout.itemSize {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to compare the sizes? Trying to understand what's going on in 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.

It is. Basically during one of the layout passes, the adjustedContentInsets change, and our itemSize depends on those contentInset values. In order to make sure the itemSize has all the update-to-date values (and so that it's the right height), we need to do this check.

extension RewardsCollectionViewController {
override func scrollViewDidScroll(_ scrollView: UIScrollView) {
self.collectionView.visibleCells.compactMap { $0 as? RewardCell }.forEach {
$0.cancelDepress()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is cancelDepress still being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh good catch. Hmm @justinswart can you describe the effect of this? I'm not seeing any issues when removing it, the "depress" action seems to cancel when you lift up your finger.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was for when you begin dragging the carousel, Danny wanted the card to lift again as you pan. I was mostly indifferent about it.

@ifbarrera ifbarrera requested a review from dusi August 13, 2019 19:15
self.pledgeButton.rac.hidden = self.viewModel.outputs.pledgeButtonHidden
self.gradientView.rac.hidden = self.viewModel.outputs.gradientHidden
Copy link
Contributor

Choose a reason for hiding this comment

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

In regards to consistency should this signal be named gradientViewHidden?

Copy link
Contributor

@dusi dusi left a comment

Choose a reason for hiding this comment

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

  1. Looks like @justinswart is OK with removing the cancelDepress action

  2. Did you see my question about ksr_insertSubviewInParent or was it missed on the conversation tab?

  3. Just noticed something weird that I wanted to double check...are we aware of the fact that hiddenPledgeHiddenConstraints (basically a getter for the array of constraints) causes a side effect by calling self.addBottomViewsMarginConstraints(with: self.layoutMarginsGuide)?
    Cc @Scollaco since he seems to be the author of this line.

Copy link
Contributor

@dusi dusi left a comment

Choose a reason for hiding this comment

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

🚢 🚢 🚢
🚢 🚢 🚢
🚢 🚢 🚢

@ifbarrera ifbarrera merged commit 4a75f5b into master Aug 14, 2019
@ifbarrera ifbarrera deleted the remove-custom-centering branch August 14, 2019 14:18
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