Fix reaction height issue (#1482)

- Fix reaction height issue - Add regression test - Fix reaction summary button color for text based reactions
element-hq · Aug 11, 2023 · 4503e6e · 4503e6e
1 parent 9a52b96
commit 4503e6e
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 8 deletions.
diff --git a/ElementX/Sources/Other/CollapsibleFlowLayout/CollapsibleReactionLayout.swift b/ElementX/Sources/Other/CollapsibleFlowLayout/CollapsibleReactionLayout.swift
@@ -141,13 +141,21 @@ struct CollapsibleReactionLayout: Layout {
     ///   - rows: The list of rows
     /// - Returns: The size render the rows
     private func sizeThatFits(proposal: ProposedViewSize, rows: [[FlowLayoutSubview]]) -> CGSize {
-        rows.enumerated().reduce(CGSize.zero) { partialResult, rowItem in
+        let sizes = rows.map { row in
+            row.map { subview in
+                subview.sizeThatFits(.unspecified)
+            }
+        }
+        let maxHeight = sizes.joined().reduce(0) { partialResult, size in
+            max(partialResult, size.height)
+        }
+
+        return sizes.enumerated().reduce(CGSize.zero) { partialResult, rowItem in
             let (rowIndex, row) = rowItem
             let rowSize = row.enumerated().reduce(CGSize.zero) { partialResult, subviewItem in
-                let (subviewIndex, subview) = subviewItem
-                let size = subview.sizeThatFits(.unspecified)
+                let (subviewIndex, size) = subviewItem
                 let horizontalSpacing = subviewIndex == 0 ? 0 : itemSpacing
-                return CGSize(width: partialResult.width + size.width + horizontalSpacing, height: max(partialResult.height, size.height))
+                return CGSize(width: partialResult.width + size.width + horizontalSpacing, height: maxHeight)
             }
             let verticalSpacing = rowIndex == 0 ? 0 : rowSpacing
             return CGSize(width: max(partialResult.width, rowSize.width), height: partialResult.height + rowSize.height + verticalSpacing)

diff --git a/ElementX/Sources/Screens/RoomScreen/View/Supplementary/ReactionsSummaryView.swift b/ElementX/Sources/Screens/RoomScreen/View/Supplementary/ReactionsSummaryView.swift
@@ -95,17 +95,22 @@ private struct ReactionSummaryButton: View {
         HStack(spacing: 4) {
             Text(reaction.displayKey)
                 .font(.compound.headingSM)
+                .foregroundColor(textColor)
             if reaction.count > 1 {
                 Text(String(reaction.count))
                     .font(.compound.headingSM)
-                    .foregroundColor(highlighted ? Color.compound.textOnSolidPrimary : Color.compound.textSecondary)
+                    .foregroundColor(textColor)
             }
         }
         .padding(.vertical, 8)
         .padding(.horizontal, 12)
         .background(highlighted ? Color.compound.bgActionPrimaryRest : .clear, in: Capsule())
         .accessibilityElement(children: .combine)
     }
+
+    var textColor: Color {
+        highlighted ? Color.compound.textOnSolidPrimary : Color.compound.textSecondary
+    }
 }
 
 private struct ReactionSummarySenderView: View {

diff --git a/UnitTests/Sources/LayoutTests/CollapsibleFlowLayoutTests.swift b/UnitTests/Sources/LayoutTests/CollapsibleFlowLayoutTests.swift
@@ -101,6 +101,37 @@ final class CollapsibleFlowLayoutTests: XCTestCase {
         XCTAssertEqual(placedViews, targetPlacements)
     }
 
+    func testHeightIsCorrectGivenASmallerAddButton() {
+        let containerSize = CGSize(width: 250, height: 400)
+        let flowLayout = CollapsibleReactionLayout(itemSpacing: 5, rowSpacing: 5, rowsBeforeCollapsible: 2)
+
+        var placedViews: [CGRect] = []
+        let placedViewsCallback = { rect in
+            placedViews.append(rect)
+        }
+
+        // Add more button with smaller initial height
+        let subviews = createReactionLayoutSubviews(with: Array(repeating: CGSize(width: 100, height: 50), count: 2),
+                                                    addMoreSize: CGSize(width: 100, height: 20),
+                                                    placedPositionCallback: placedViewsCallback)
+        let subviewsMock = LayoutSubviewsMock(subviews: subviews)
+        var a: () = ()
+        let size = flowLayout.sizeThatFits(proposal: ProposedViewSize(containerSize), subviews: subviewsMock, cache: &a)
+
+        XCTAssertEqual(size, CGSize(width: 205, height: 105))
+        flowLayout.placeSubviews(in: CGRect(origin: .zero, size: size), proposal: ProposedViewSize(containerSize), subviews: subviewsMock, cache: &a)
+
+        let targetPlacements: [CGRect] = [
+            CGRect(x: 0, y: 25, width: 100, height: 50),
+            CGRect(x: 105, y: 25, width: 100, height: 50),
+            // Add more button with matching height
+            CGRect(x: 0, y: 80, width: 100, height: 50),
+            // Expand/Collapse button is hidden
+            CGRect(x: -10000, y: -10000, width: 0, height: 0)
+        ]
+        XCTAssertEqual(placedViews, targetPlacements)
+    }
+
     func testFlowLayoutEmptyState() {
         let containerSize = CGSize(width: 250, height: 400)
         let flowLayout = CollapsibleReactionLayout(itemSpacing: 5, rowSpacing: 5, rowsBeforeCollapsible: 2)
@@ -125,16 +156,19 @@ final class CollapsibleFlowLayoutTests: XCTestCase {
         XCTAssertEqual(placedViews, targetPlacements)
     }
 
-    func createReactionLayoutSubviews(with sizes: [CGSize], placedPositionCallback: @escaping (CGRect) -> Void) -> [LayoutSubviewMock] {
+    func createReactionLayoutSubviews(with sizes: [CGSize],
+                                      expandCollapseSize: CGSize = CGSize(width: 100, height: 50),
+                                      addMoreSize: CGSize = CGSize(width: 100, height: 50),
+                                      placedPositionCallback: @escaping (CGRect) -> Void) -> [LayoutSubviewMock] {
         sizes.map { size in
             LayoutSubviewMock(size: size,
                               layoutValues: [String(describing: ReactionLayoutItemType.self): ReactionLayoutItem.reaction],
                               placedPositionCallback: placedPositionCallback)
         } + [
-            LayoutSubviewMock(size: CGSize(width: 100, height: 50),
+            LayoutSubviewMock(size: expandCollapseSize,
                               layoutValues: [String(describing: ReactionLayoutItemType.self): ReactionLayoutItem.expandCollapse],
                               placedPositionCallback: placedPositionCallback),
-            LayoutSubviewMock(size: CGSize(width: 100, height: 50),
+            LayoutSubviewMock(size: addMoreSize,
                               layoutValues: [String(describing: ReactionLayoutItemType.self): ReactionLayoutItem.addMore],
                               placedPositionCallback: placedPositionCallback)
         ]