Backpack PoC for Search Controls GC#2504
Backpack PoC for Search Controls GC#2504Soheil Novinfard (novinfard) wants to merge 6 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces new search controls components and enhancements as a Proof of Concept for Search Controls GC. The changes add vertical layout support for navigation tabs, new styling options for dark backgrounds, docking functionality for stacked search inputs, and a new swap button component.
Changes:
- Introduces new
BPKSwapcomponent - a circular button with animated rotation for swapping values - Adds vertical item alignment and
onDarkAlternatestyle toBPKNavigationTabGroup - Enhances
BPKSwitchwith.onContraststyle parameter for dark backgrounds - Adds docking support to
BPKSearchInputSummaryfor stacked inputs with selective corner rounding - Adds
.surfaceContraststyle toBPKSegmentedControland removes built-in padding
Reviewed changes
Copilot reviewed 12 out of 21 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
Backpack-SwiftUI/Swap/Classes/BPKSwap.swift |
New circular swap button component with rotation animation and three style variants |
Backpack-SwiftUI/Swap/README.md |
Comprehensive documentation for BPKSwap including usage examples and style guide |
Backpack-SwiftUI/Switch/Classes/BPKSwitch.swift |
Adds style parameter with custom toggle style implementation for dark background support |
Backpack-SwiftUI/SegmentedControl/Classes/BPKSegmentedControl.swift |
Removes built-in padding and adds surfaceContrast style |
Backpack-SwiftUI/SearchInputSummary/Classes/BPKSearchInputSummary.swift |
Adds docking modifier for stacked inputs, makes clearAction optional, adds inputMinHeight modifier |
Backpack-SwiftUI/SearchInputSummary/Classes/BPKSearchInputSummaryStyle.swift |
Defines Docking enum with corner radius logic for stacked inputs |
Backpack-SwiftUI/NavigationTab/Classes/BPKNavigationTabGroup.swift |
Adds itemAlignment parameter and onDarkAlternate style support |
Backpack-SwiftUI/NavigationTab/Classes/BPKNavigationTab.swift |
Implements vertical and horizontal content layouts based on itemAlignment |
Backpack-SwiftUI/NavigationTab/Classes/NavigationTabStyle.swift |
Updates styling logic for vertical alignment and onDarkAlternate style |
Backpack-SwiftUI/NavigationTab/Classes/BPKNavigationTab+Style.swift |
Defines ItemAlignment enum and onDarkAlternate style case |
Backpack-SwiftUI/NavigationTab/README.md |
Comprehensive documentation with examples and screenshots |
Backpack-SwiftUI/Tests/NavigationTab/BPKNavigationTabGroupTests.swift |
Adds snapshot tests for new styles and vertical alignment |
| Snapshot files | New snapshot test images for NavigationTab variations |
| @@ -0,0 +1,143 @@ | |||
| # Backpack-SwiftUI/NavigationTab | |||
|
|
|||
| [](hhttps://cocoapods.org/pods/Backpack-SwiftUI) | |||
There was a problem hiding this comment.
Typo in the URL: "hhttps" should be "https". This will result in a broken link in the documentation.
| [](hhttps://cocoapods.org/pods/Backpack-SwiftUI) | |
| [](https://cocoapods.org/pods/Backpack-SwiftUI) |
| @@ -67,7 +67,6 @@ public struct BPKSegmentedControl: View { | |||
| .background(Color(style.bgColor)) | |||
| .animation(.spring(duration: 0.25), value: selectedIndex) | |||
| .cornerRadius(8) | |||
There was a problem hiding this comment.
The removal of .padding() from the BPKSegmentedControl changes its default layout behavior. This is a breaking API change that could affect existing consumers who rely on the built-in padding. If this is intentional (to give consumers more control over layout), it should be clearly documented and potentially mentioned in release notes or migration guides. Consider whether this breaking change is appropriate for a PoC or if it should maintain backward compatibility.
| .cornerRadius(8) | |
| .cornerRadius(8) | |
| .padding() |
| /* | ||
| * Backpack - Skyscanner's Design System | ||
| * | ||
| * Copyright 2018 Skyscanner Ltd | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| import SwiftUI | ||
|
|
||
| /// A circular button with a swap icon that rotates 180 degrees when tapped. | ||
| /// | ||
| /// Use `BPKSwap` to provide a swap action, typically used to swap origin and destination | ||
| /// in search controls or similar use cases. | ||
| /// | ||
| /// The button animates the icon with a 180-degree rotation on each tap to provide | ||
| /// visual feedback to the user. | ||
| /// | ||
| /// Example usage: | ||
| /// ```swift | ||
| /// BPKSwap(accessibilityLabel: "Swap origin and destination") { | ||
| /// // Perform swap action | ||
| /// } | ||
| /// ``` | ||
| public struct BPKSwap: View { | ||
| private let style: Style | ||
| private let accessibilityLabel: String | ||
| private let action: () -> Void | ||
|
|
||
| @State private var rotationAngle: Double = 0 | ||
| @State private var rotateClockwise: Bool = false | ||
|
|
||
| /// Creates a new swap button. | ||
| /// | ||
| /// - Parameters: | ||
| /// - style: The visual style of the button. Defaults to `.canvasContrast`. | ||
| /// - accessibilityLabel: A localized string that describes the button's action. | ||
| /// - action: The closure to execute when the user taps the button. | ||
| public init( | ||
| style: Style = .canvasContrast, | ||
| accessibilityLabel: String, | ||
| action: @escaping () -> Void | ||
| ) { | ||
| self.style = style | ||
| self.accessibilityLabel = accessibilityLabel | ||
| self.action = action | ||
| } | ||
|
|
||
| public var body: some View { | ||
| Button { | ||
| withAnimation(.easeInOut(duration: 0.3)) { | ||
| rotationAngle += rotateClockwise ? 180 : -180 | ||
| } | ||
| rotateClockwise.toggle() | ||
| action() | ||
| } label: { | ||
| BPKIconView(.swapVertical, size: .small) | ||
| .foregroundColor(style.iconColor) | ||
| .rotationEffect(.degrees(rotationAngle)) | ||
| } | ||
| .frame(width: Constants.containerSize, height: Constants.containerSize) | ||
| .background(Color(style.backgroundColor)) | ||
| .clipShape(Circle()) | ||
| .overlay( | ||
| Circle() | ||
| .stroke(Color(style.borderColor), lineWidth: Constants.borderWidth) | ||
| ) | ||
| .accessibilityLabel(accessibilityLabel) | ||
| .buttonStyle(NoOpacityButtonStyle()) | ||
| } | ||
|
|
||
| private enum Constants { | ||
| static let containerSize: CGFloat = 36 | ||
| static let borderWidth: CGFloat = 2 | ||
| } | ||
| } | ||
|
|
||
| // MARK: - Button Style | ||
|
|
||
| private struct NoOpacityButtonStyle: ButtonStyle { | ||
| func makeBody(configuration: Configuration) -> some View { | ||
| configuration.label | ||
| } | ||
| } | ||
|
|
||
| // MARK: - Style | ||
|
|
||
| public extension BPKSwap { | ||
| /// The visual style for `BPKSwap`. | ||
| enum Style { | ||
| /// Use on canvas default backgrounds. | ||
| /// Gray background with white border. | ||
| case canvasDefault | ||
|
|
||
| /// Use on canvas contrast backgrounds. | ||
| /// White background with gray border. | ||
| case canvasContrast | ||
|
|
||
| /// Use on surface contrast (dark) backgrounds. | ||
| /// White background with dark primary border. | ||
| case surfaceContrast | ||
|
|
||
| var backgroundColor: BPKColor { | ||
| switch self { | ||
| case .canvasDefault: | ||
| return .canvasContrastColor | ||
| case .canvasContrast, .surfaceContrast: | ||
| return .surfaceDefaultColor | ||
| } | ||
| } | ||
|
|
||
| var borderColor: BPKColor { | ||
| switch self { | ||
| case .canvasDefault: | ||
| return .canvasColor | ||
| case .canvasContrast: | ||
| return .canvasContrastColor | ||
| case .surfaceContrast: | ||
| return .corePrimaryColor | ||
| } | ||
| } | ||
|
|
||
| var iconColor: BPKColor { | ||
| switch self { | ||
| case .canvasDefault, .canvasContrast: | ||
| return .textPrimaryColor | ||
| case .surfaceContrast: | ||
| return .corePrimaryColor | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // MARK: - Previews | ||
|
|
||
| struct BPKSwap_Previews: PreviewProvider { | ||
| static var previews: some View { | ||
| VStack(spacing: .xl) { | ||
| // Canvas Default | ||
| VStack(alignment: .leading, spacing: .md) { | ||
| BPKText("Canvas Default", style: .caption) | ||
| BPKSwap(style: .canvasDefault, accessibilityLabel: "Swap") { } | ||
| } | ||
| .padding() | ||
| .background(.canvasColor) | ||
|
|
||
| // Canvas Contrast | ||
| VStack(alignment: .leading, spacing: .md) { | ||
| BPKText("Canvas Contrast", style: .caption) | ||
| BPKSwap(style: .canvasContrast, accessibilityLabel: "Swap") { } | ||
| } | ||
| .padding() | ||
| .background(.canvasContrastColor) | ||
|
|
||
| // Surface Contrast | ||
| VStack(alignment: .leading, spacing: .md) { | ||
| BPKText("Surface Contrast", style: .caption) | ||
| .foregroundColor(.textOnDarkColor) | ||
| BPKSwap(style: .surfaceContrast, accessibilityLabel: "Swap") { } | ||
| } | ||
| .padding() | ||
| .background(.surfaceContrastColor) | ||
| } | ||
| .padding() | ||
| } | ||
| } |
There was a problem hiding this comment.
The new BPKSwap component lacks test coverage. Based on the repository's testing practices (e.g., BPKSwitch, BPKNavigationTab), components should have snapshot tests covering all visual states and styles. Please add snapshot tests for all three styles (canvasDefault, canvasContrast, surfaceContrast) in both on and off states, following the pattern seen in other component tests.
| public init( | ||
| isOn: Binding<Bool>, | ||
| style: Style = .default, | ||
| @ViewBuilder content: () -> Content | ||
| ) { | ||
| self._isOn = isOn | ||
| self.style = style | ||
| self.content = content() | ||
| } |
There was a problem hiding this comment.
The new .onContrast style parameter for BPKSwitch is not tested. The existing tests in BPKSwitchTests.swift only test the default style. Per the Backpack iOS Component Standards, components should have comprehensive snapshot tests for all visual states and variations. Please add snapshot tests for the .onContrast style in both on and off states, similar to the pattern used in NavigationTab tests.
| static let surfaceContrast = Self( | ||
| bgColor: .segmentedControlSurfaceContrastColor, | ||
| textColor: .textOnDarkColor, | ||
| selectedBgColor: .segmentedControlSurfaceContrastOnColor, | ||
| selectedTextColor: .textOnDarkColor | ||
| ) | ||
| } |
There was a problem hiding this comment.
The new .surfaceContrast style for BPKSegmentedControl lacks test coverage. Following the repository's testing conventions (see other component tests), snapshot tests should be added to verify the visual appearance of this new style in all states. Please add snapshot tests covering this new style variation.
| public func docking(_ docking: Docking) -> BPKSearchInputSummary { | ||
| var result = self | ||
| result.docking = docking | ||
| return result | ||
| } | ||
|
|
||
| /// Sets the minimum height for the search input. | ||
| /// - Parameter height: The minimum height in points. Default is 48. | ||
| /// - Returns: A modified search input summary with the specified minimum height | ||
| public func inputMinHeight(_ height: CGFloat) -> BPKSearchInputSummary { | ||
| var result = self | ||
| result.minHeight = height | ||
| return result | ||
| } |
There was a problem hiding this comment.
The new docking functionality (.docking() modifier and Docking enum) in BPKSearchInputSummary lacks test coverage. The repository follows comprehensive testing practices with snapshot tests for component variations. Please add snapshot tests covering the different docking positions (.float, .top, .middle, .bottom) to ensure correct visual rendering of rounded corners in stacked configurations.
| placeholder: String = "", | ||
| inputPrefix: InputPrefix? = nil, | ||
| clearAction: ClearAction, | ||
| clearAction: ClearAction? = nil, |
There was a problem hiding this comment.
The clearAction parameter is now optional, but there's no validation or handling for the case where clearAction is nil and text is not empty. Consider whether this is the intended behavior - previously clearAction was required, suggesting it was always expected to be present. If making it optional is intentional for cases like docked search inputs without clear buttons, this is acceptable, but please ensure the behavior is clearly documented.
Introduce a withBorder Bool to BPKSearchInputSummary (default true) and wire it through the initializer. dockingBorder now selects a Color based on withBorder and highlight state (accent/line colors, or clear when withBorder is false) and uses that color for the stroke instead of a fixed clear color. This allows callers to disable the visible border while preserving existing highlight behavior by default.
Backpack PoC for Search Controls GC
Remember to include the following changes:
README.mdBackpack.hheader fileIf you are curious about how we review, please read through the code review guidelines