Refine top and bottom item anchor calculation (#129)

* Refine top and bottom item anchor calculation * Fix build
airbnb · May 2, 2024 · 7655494 · 7655494
1 parent f9b27b7
commit 7655494
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 76 deletions.
diff --git a/.github/workflows/swift.yml b/.github/workflows/swift.yml
@@ -1,4 +1,4 @@
-name: Swift
+name: CI
 
 on:
   push:
@@ -8,17 +8,14 @@ on:
 
 jobs:
   build:
-
-    runs-on: macos-latest
-
+    runs-on: macos-13
     strategy:
       matrix:
-        destination: ['platform=iOS Simulator,OS=11.0,name=iPhone 8', 'platform=iOS Simulator,OS=16.2,name=iPhone 14']
-
+        xcode:
+        - '15.0' # Swift 5.9
     steps:
-    - uses: actions/checkout@v2
-    - name: Build
-      run: xcodebuild clean build -scheme MagazineLayout
-    - name: Run tests
-      run: xcodebuild clean test -project MagazineLayout.xcodeproj -scheme MagazineLayout -destination "name=iPhone 14,OS=16.2"
-
+      - uses: actions/checkout@v4
+      - name: Build
+        run: xcodebuild clean build -scheme MagazineLayout -destination "generic/platform=iOS Simulator"
+      - name: Run tests
+        run: xcodebuild clean test -project MagazineLayout.xcodeproj -scheme MagazineLayout -destination "name=iPhone 14,OS=17.2"
diff --git a/MagazineLayout/LayoutCore/Types/TargetContentOffsetAnchor.swift b/MagazineLayout/LayoutCore/Types/TargetContentOffsetAnchor.swift
@@ -22,8 +22,8 @@ import UIKit
 enum TargetContentOffsetAnchor: Equatable {
   case top
   case bottom
-  case topItem(id: String, itemEdge: ItemEdge, distanceFromTop: CGFloat)
-  case bottomItem(id: String, itemEdge: ItemEdge, distanceFromBottom: CGFloat)
+  case topItem(id: String, distanceFromTop: CGFloat)
+  case bottomItem(id: String, distanceFromBottom: CGFloat)
 
   static func targetContentOffsetAnchor(
     verticalLayoutDirection: MagazineLayoutVerticalLayoutDirection,
@@ -65,37 +65,19 @@ enum TargetContentOffsetAnchor: Equatable {
         return .top
       case .inMiddle, .atBottom:
         let top = bounds.minY + topInset
-        let topDistanceFromTop = firstVisibleItemFrame.value(for: .top) - top
-        let bottomDistanceFromTop = firstVisibleItemFrame.value(for: .bottom) - top
-        if abs(topDistanceFromTop) < abs(bottomDistanceFromTop) {
-          return .topItem(
-            id: firstVisibleItemID,
-            itemEdge: .top,
-            distanceFromTop: topDistanceFromTop.alignedToPixel(forScreenWithScale: scale))
-        } else {
-          return .topItem(
-            id: firstVisibleItemID,
-            itemEdge: .bottom,
-            distanceFromTop: bottomDistanceFromTop.alignedToPixel(forScreenWithScale: scale))
-        }
+        let distanceFromTop = firstVisibleItemFrame.minY - top
+        return .topItem(
+          id: firstVisibleItemID,
+          distanceFromTop: distanceFromTop.alignedToPixel(forScreenWithScale: scale))
       }
     case .bottomToTop:
       switch position {
       case .atTop, .inMiddle:
         let bottom = bounds.maxY - bottomInset
-        let topDistanceFromBottom = lastVisibleItemFrame.value(for: .top) - bottom
-        let bottomDistanceFromBottom = lastVisibleItemFrame.value(for: .bottom) - bottom
-        if abs(topDistanceFromBottom) < abs(bottomDistanceFromBottom) {
-          return .bottomItem(
-            id: lastVisibleItemID,
-            itemEdge: .top,
-            distanceFromBottom: topDistanceFromBottom.alignedToPixel(forScreenWithScale: scale))
-        } else {
-          return .bottomItem(
-            id: lastVisibleItemID,
-            itemEdge: .bottom,
-            distanceFromBottom: bottomDistanceFromBottom.alignedToPixel(forScreenWithScale: scale))
-        }
+        let distanceFromBottom = lastVisibleItemFrame.maxY - bottom
+        return .bottomItem(
+          id: lastVisibleItemID,
+          distanceFromBottom: distanceFromBottom.alignedToPixel(forScreenWithScale: scale))
       case .atBottom:
         return .bottom
       }
@@ -121,44 +103,22 @@ enum TargetContentOffsetAnchor: Equatable {
     case .bottom:
       return maxYOffset
 
-    case .topItem(let id, let itemEdge, let distanceFromTop):
+    case .topItem(let id, let distanceFromTop):
       guard let indexPath = indexPathForItemID(id) else { return bounds.minY }
       let itemFrame = frameForItemAtIndexPath(indexPath)
-      let proposedYOffset = itemFrame.value(for: itemEdge) - topInset - distanceFromTop
+      let proposedYOffset = itemFrame.minY - topInset - distanceFromTop
       return min(max(proposedYOffset, minYOffset), maxYOffset) // Clamp between minYOffset...maxYOffset
 
-    case .bottomItem(let id, let itemEdge, let distanceFromBottom):
+    case .bottomItem(let id, let distanceFromBottom):
       guard let indexPath = indexPathForItemID(id) else { return bounds.minY }
       let itemFrame = frameForItemAtIndexPath(indexPath)
-      let proposedYOffset = itemFrame.value(for: itemEdge) - bounds.height + bottomInset - distanceFromBottom
+      let proposedYOffset = itemFrame.maxY - bounds.height + bottomInset - distanceFromBottom
       return min(max(proposedYOffset, minYOffset), maxYOffset) // Clamp between minYOffset...maxYOffset
     }
   }
 
 }
 
-// MARK: ItemEdge
-
-enum ItemEdge {
-  case top
-  case bottom
-}
-
-// MARK: CGRect + Item Edge Value
-
-private extension CGRect {
-
-  func value(for itemEdge: ItemEdge) -> CGFloat {
-    switch itemEdge {
-    case .top:
-      return minY
-    case .bottom:
-      return maxY
-    }
-  }
-
-}
-
 // MARK: - Position
 
 private enum Position {

diff --git a/MagazineLayout/Public/MagazineLayout.swift b/MagazineLayout/Public/MagazineLayout.swift
@@ -1261,9 +1261,8 @@ public final class MagazineLayout: UICollectionViewLayout {
     bottomInset: CGFloat)
     -> TargetContentOffsetAnchor?
   {
-    let insetBounds = bounds.inset(by: .init(top: topInset, left: 0, bottom: bottomInset, right: 0))
     var visibleItemLocationFramePairs = [ElementLocationFramePair]()
-    for itemLocationFramePair in modelState.itemLocationFramePairs(forItemsIn: insetBounds) {
+    for itemLocationFramePair in modelState.itemLocationFramePairs(forItemsIn: bounds) {
       visibleItemLocationFramePairs.append(itemLocationFramePair)
     }
     visibleItemLocationFramePairs.sort { $0.elementLocation < $1.elementLocation }

diff --git a/Tests/TargetContentOffsetAnchorTests.swift b/Tests/TargetContentOffsetAnchorTests.swift
@@ -48,7 +48,7 @@ final class TargetContentOffsetAnchorTests: XCTestCase {
       lastVisibleItemID: "6",
       firstVisibleItemFrame: CGRect(x: 0, y: 560, width: 300, height: 20),
       lastVisibleItemFrame: CGRect(x: 0, y: 800, width: 300, height: 20))
-    XCTAssert(anchor == .topItem(id: "2", itemEdge: .top, distanceFromTop: 10))
+    XCTAssert(anchor == .topItem(id: "2", distanceFromTop: 10))
   }
 
   func testAnchor_TopToBottom_ScrolledToBottom() throws {
@@ -63,7 +63,7 @@ final class TargetContentOffsetAnchorTests: XCTestCase {
       lastVisibleItemID: "10",
       firstVisibleItemFrame: CGRect(x: 0, y: 1700, width: 300, height: 20),
       lastVisibleItemFrame: CGRect(x: 0, y: 1950, width: 300, height: 20))
-    XCTAssert(anchor == .topItem(id: "6", itemEdge: .top, distanceFromTop: 20))
+    XCTAssert(anchor == .topItem(id: "6", distanceFromTop: 20))
   }
 
   func testAnchor_TopToBottom_SmallContentHeight() throws {
@@ -95,7 +95,7 @@ final class TargetContentOffsetAnchorTests: XCTestCase {
       lastVisibleItemID: "4",
       firstVisibleItemFrame: CGRect(x: 0, y: 0, width: 300, height: 20),
       lastVisibleItemFrame: CGRect(x: 0, y: 290, width: 300, height: 20))
-    XCTAssert(anchor == .bottomItem(id: "4", itemEdge: .bottom, distanceFromBottom: -10))
+    XCTAssert(anchor == .bottomItem(id: "4", distanceFromBottom: -10))
   }
 
   func testAnchor_BottomToTop_ScrolledToMiddle() throws {
@@ -110,7 +110,7 @@ final class TargetContentOffsetAnchorTests: XCTestCase {
       lastVisibleItemID: "6",
       firstVisibleItemFrame: CGRect(x: 0, y: 560, width: 300, height: 20),
       lastVisibleItemFrame: CGRect(x: 0, y: 800, width: 300, height: 20))
-    XCTAssert(anchor == .bottomItem(id: "6", itemEdge: .bottom, distanceFromBottom: -50))
+    XCTAssert(anchor == .bottomItem(id: "6", distanceFromBottom: -50))
   }
 
   func testAnchor_BottomToTop_ScrolledToBottom() throws {
@@ -158,7 +158,7 @@ final class TargetContentOffsetAnchorTests: XCTestCase {
   }
 
   func testOffset_TopToBottom_ScrolledToMiddle() {
-    let anchor = TargetContentOffsetAnchor.topItem(id: "2", itemEdge: .top, distanceFromTop: 10)
+    let anchor = TargetContentOffsetAnchor.topItem(id: "2", distanceFromTop: 10)
     let offset = anchor.yOffset(
       topInset: 50,
       bottomInset: 30,
@@ -170,7 +170,7 @@ final class TargetContentOffsetAnchorTests: XCTestCase {
   }
 
   func testOffset_TopToBottom_ScrolledToBottom() {
-    let anchor = TargetContentOffsetAnchor.topItem(id: "2", itemEdge: .top, distanceFromTop: 10)
+    let anchor = TargetContentOffsetAnchor.topItem(id: "2", distanceFromTop: 10)
     let offset = anchor.yOffset(
       topInset: 50,
       bottomInset: 30,
@@ -184,7 +184,7 @@ final class TargetContentOffsetAnchorTests: XCTestCase {
   // MARK: Bottom-to-Top Target Content Offset Tests
 
   func testOffset_BottomToTop_ScrolledToTop() {
-    let anchor = TargetContentOffsetAnchor.bottomItem(id: "4", itemEdge: .bottom, distanceFromBottom: -10)
+    let anchor = TargetContentOffsetAnchor.bottomItem(id: "4", distanceFromBottom: -10)
     let offset = anchor.yOffset(
       topInset: 50,
       bottomInset: 30,
@@ -196,7 +196,7 @@ final class TargetContentOffsetAnchorTests: XCTestCase {
   }
 
   func testOffset_BottomToTop_ScrolledToMiddle() {
-    let anchor = TargetContentOffsetAnchor.bottomItem(id: "6", itemEdge: .bottom, distanceFromBottom: -50)
+    let anchor = TargetContentOffsetAnchor.bottomItem(id: "6", distanceFromBottom: -50)
     let offset = anchor.yOffset(
       topInset: 50,
       bottomInset: 30,