[DO-NOT-MERGE] search control GC PoC - #2609
Conversation
Generated by 🚫 Danger Kotlin against 2b4a16b |
There was a problem hiding this comment.
Pull request overview
This PR appears to be a PoC/iteration for “search control” UI work in Backpack Compose, adding a new BpkSwapButton component, extending BpkSearchInputSummary to support configurable corner rounding, and enhancing a few supporting components/demos.
Changes:
- Add
BpkSwapButton(Compose component), demo story, README, and snapshot tests. - Add
BpkSearchInputSummaryRoundingand thread rounding through to the underlying text field shape. - Extend
BpkSwitchwith optional trailing icon + click handling, and addcolorsupport; allowBpkDividercolor override.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| backpack-compose/src/main/kotlin/net/skyscanner/backpack/compose/textfield/internal/BpkTextFieldImpl.kt | Threads a new rounding parameter through to the textfield shape. |
| backpack-compose/src/main/kotlin/net/skyscanner/backpack/compose/switch/BpkSwitch.kt | Adds optional icon + click handling and a color override for switch label content. |
| backpack-compose/src/main/kotlin/net/skyscanner/backpack/compose/swapbutton/README.md | New component documentation for BpkSwapButton. |
| backpack-compose/src/main/kotlin/net/skyscanner/backpack/compose/swapbutton/BpkSwapButton.kt | New BpkSwapButton component implementation. |
| backpack-compose/src/main/kotlin/net/skyscanner/backpack/compose/searchinputsummary/BpkSearchInputSummary.kt | Adds rounding API for BpkSearchInputSummary. |
| backpack-compose/src/main/kotlin/net/skyscanner/backpack/compose/divider/BpkDivider.kt | Adds an optional color parameter for divider customization. |
| app/src/test/java/net/skyscanner/backpack/compose/swapbutton/BpkSwapButtonTest.kt | Adds snapshot tests and a basic click interaction test for swap button. |
| app/src/main/java/net/skyscanner/backpack/demo/compose/SwitchStory.kt | Demonstrates the new switch icon API. |
| app/src/main/java/net/skyscanner/backpack/demo/compose/SwapButtonStory.kt | Adds demo story for the new swap button component. |
| app/src/main/java/net/skyscanner/backpack/demo/compose/BpkSearchInputSummaryStory.kt | Adds a “corner rounding” demo and threads rounding through the story helper. |
| app/src/main/java/net/skyscanner/backpack/demo/components/SwapButtonComponent.kt | Registers component marker for demo/story tooling. |
| .idea/deviceManager.xml | Adds IDE configuration file (should not be committed). |
Files not reviewed (1)
- .idea/deviceManager.xml: Language not supported
Comments suppressed due to low confidence (1)
app/src/main/java/net/skyscanner/backpack/demo/compose/BpkSearchInputSummaryStory.kt:262
SearchInputSummaryStorystill takes anameparameter, but it’s no longer used (the heading label was removed). Please remove the parameter or restore using it so we don’t keep dead/unused API in the demo code.
internal fun SearchInputSummaryStory(
name: String,
modifier: Modifier = Modifier,
searchInputSummaryModifier: Modifier = Modifier,
| tint = color, | ||
| ) |
There was a problem hiding this comment.
tint = color is always applied to the icon. With the default color = Color.Unspecified, this bypasses LocalContentColor from BpkToggleableContent and can render the icon with its intrinsic (often black) color (notably wrong in DarkMode). Consider using LocalContentColor.current when color is Unspecified, or only overriding tint when a real color is provided.
| interactionSource: MutableInteractionSource = remember { MutableInteractionSource() }, | ||
| icon: BpkIcon? = null, | ||
| onIconClick: (() -> Unit)? = null, | ||
| content: @Composable RowScope.(Boolean) -> Unit, |
There was a problem hiding this comment.
The icon and onIconClick parameters on the overload that accepts a content lambda are not used inside the function body, making the public API confusing. Either remove them from this overload or integrate them into the layout/contract so callers can actually get the icon behavior.
| if (enabled) { | ||
| role = Role.Button | ||
| } | ||
| }, |
There was a problem hiding this comment.
The semantics currently set role only when enabled, and do not mark the component as disabled when enabled = false. For accessibility, set the role consistently and add disabled() semantics when not enabled so screen readers announce the disabled state.
| BpkSwapButton includes comprehensive tests covering: | ||
|
|
||
| - Visual snapshots in light and dark themes | ||
| - Interaction behavior (click handling) | ||
| - Disabled state rendering | ||
| - Accessibility semantics |
There was a problem hiding this comment.
The README claims "comprehensive tests" including accessibility semantics, but the added test class only does snapshots plus a click without assertions (and no semantics checks). Please either add the stated test coverage or tone down this section so it matches what is actually tested.
| BpkSwapButton includes comprehensive tests covering: | |
| - Visual snapshots in light and dark themes | |
| - Interaction behavior (click handling) | |
| - Disabled state rendering | |
| - Accessibility semantics | |
| BpkSwapButton currently has automated tests covering: | |
| - Visual snapshots in light and dark themes | |
| - Basic interaction behavior (click handling) |
| @Test | ||
| @Variants(BpkTestVariant.Default, BpkTestVariant.DarkMode) | ||
| fun canvasDefault() { |
There was a problem hiding this comment.
Snapshot tests for new components typically include an RTL variant in addition to Default/DarkMode. Adding BpkTestVariant.Rtl coverage here will help catch layout/RTL regressions early.
| name = "Croner", | ||
| inputText = "", | ||
| inputHint = "Thurs 9 May - Fri 29 May, 2025", | ||
| prefix = Prefix.Icon(BpkIcon.Calendar), | ||
| rounding = BpkSearchInputSummaryRounding.NoRoundedCorners, |
There was a problem hiding this comment.
Typo in the story name: "Croner" should be "Corner".
| name = "Croner", | ||
| inputText = "", | ||
| inputHint = "2 adults, 1 child", | ||
| prefix = Prefix.Icon(BpkIcon.Family), | ||
| rounding = BpkSearchInputSummaryRounding.BottomCorners, |
There was a problem hiding this comment.
Typo in the story name: "Croner" should be "Corner".
| // BpkIcon.InformationCircle would be used here when the icon extensions are properly exposed | ||
| // For now, here's the API usage example: |
There was a problem hiding this comment.
The comment about icon extensions "not properly exposed" is misleading now that BpkIcon.InformationCircle is being used via the tokens import. Please remove or update this comment to reflect the current state.
| // BpkIcon.InformationCircle would be used here when the icon extensions are properly exposed | |
| // For now, here's the API usage example: | |
| // Example of a switch with an info icon. |
| <project version="4"> | ||
| <component name="DeviceTable"> | ||
| <option name="columnSorters"> | ||
| <list> | ||
| <ColumnSorterState> | ||
| <option name="column" value="Name" /> | ||
| <option name="order" value="ASCENDING" /> | ||
| </ColumnSorterState> | ||
| </list> | ||
| </option> | ||
| </component> | ||
| </project> No newline at end of file |
There was a problem hiding this comment.
IDE-generated .idea/deviceManager.xml should not be committed to the repository. Please remove this file from the PR and add it to .gitignore if it isn’t already ignored.
| <project version="4"> | |
| <component name="DeviceTable"> | |
| <option name="columnSorters"> | |
| <list> | |
| <ColumnSorterState> | |
| <option name="column" value="Name" /> | |
| <option name="order" value="ASCENDING" /> | |
| </ColumnSorterState> | |
| </list> | |
| </option> | |
| </component> | |
| </project> | |
| <!-- | |
| This file is intentionally left without IDE-specific configuration. | |
| `.idea/deviceManager.xml` is generated by the IDE and should not be tracked in version control. | |
| Please remove this file from Git (e.g. `git rm --cached .idea/deviceManager.xml`) | |
| and add `.idea/deviceManager.xml` (or the `.idea` directory) to `.gitignore`. | |
| --> |
Generated by 🚫 Danger Kotlin against 479aadc |
Generated by 🚫 Danger Kotlin against c8c324f |
Generated by 🚫 Danger Kotlin against b7eac94 |
Generated by 🚫 Danger Kotlin against 8a93615 |
Generated by 🚫 Danger Kotlin against d2e72fd |
Generated by 🚫 Danger Kotlin against 8b9cecc |
Generated by 🚫 Danger Kotlin against 767b539 |
Generated by 🚫 Danger Kotlin against 7d084db |
Generated by 🚫 Danger Kotlin against 72c0f11 |
Generated by 🚫 Danger Kotlin against fa06550 |
Generated by 🚫 Danger Kotlin against feabf92 |
Generated by 🚫 Danger Kotlin against 41de577 |
Co-authored-by: Copilot <Copilot@github.com>
Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
…zation - Add horizontalPadding, contentPadding, and minHeight parameters to BpkSearchInputSummary - Support iconTint and textTint for color customization - Update default minHeight to 56dp (BpkSpacing.Xxl + BpkSpacing.Base) - Update CornerExample to match Figma design with grouped search inputs - Remove spacing between fields for seamless grouped appearance - Update text hints to match Figma mockup (London Heathrow, Barcelona, dates, passengers) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
- Create BpkSwitchStyle sealed class with Default and OnContrast variants - Add style parameter to all BpkSwitch overloads - Implement OnContrast colors for dark background compatibility - Add OnContrastUncheckedSwitchExample and OnContrastCheckedSwitchExample to SwitchStory - OnContrast uses lighter thumb color and textOnDark for better contrast on dark backgrounds Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
- Create BpkNavigationTabGroupItemAlignment enum with Horizontal and Vertical options - Add itemAlignment parameter to control layout direction (defaults to Horizontal) - Add tabBackgroundColor parameter to customize tab background - Add tabWidth and tabHeight parameters for custom tab sizing - Update BpkNavigationTabGroupImpl to use LazyRow for horizontal and LazyColumn for vertical - Update BpkNavigationTab to accept and apply custom colors and sizing - Add NavigationTabGroupVerticalStory with vertical alignment matching Figma design - Vertical story uses 4 tabs (Flights, Hotels, Cars, Packages) on surfaceContrast background Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
… below - Add isVertical parameter to BpkNavigationTabImpl - Pass itemAlignment through NavigationTabItem to BpkNavigationTab - Implement Column layout for vertical alignment with icon centered on top - Text positioned below icon with proper spacing using BpkSpacing.Sm - Use consistent padding (BpkSpacing.Md) for vertical layout - Maintain horizontal layout behavior when isVertical is false - Center alignment for both icon and text in vertical mode Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
- Remove custom backgroundColor, width, height parameters from BpkNavigationTab - Simplify BpkNavigationTab to accept only isVertical flag for layout control - Update BpkNavigationTabGroupImpl to pass isVertical flag instead of individual params - Use heightIn/widthIn for flexible sizing in vertical layout (56dp min height, 88dp min width) - Adjust vertical padding and horizontal padding for vertical tabs - Set icon top padding to 1.dp for tighter spacing between icon and text - Maintain consistent styling and spacing across both vertical and horizontal layouts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
- Remove unused tabBackgroundColor, tabWidth, tabHeight from BpkNavigationTabGroup - Update BpkNavigationTabGroupImpl to pass only itemAlignment flag - Simplify NavigationTabItem function signature - Update NavigationTabGroupVerticalStory to use standard configuration - Cleanup parameter passing in both Horizontal and Vertical branches Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
41de577 to
51b24ef
Compare
Generated by 🚫 Danger Kotlin against 51b24ef |
- Update androidPlugin from 8.13.2 to 9.0.1 - Update lint and androidSdkCommon to 32.0.1 (formula: X+23 for AGP version X) - Add android.builtInKotlin=true to gradle.properties (required for AGP 9.0) - Add android.newDsl=false to gradle.properties (temporary compatibility flag) - Update extension imports to AGP 9.0 DSL: ApplicationExtension, LibraryExtension - Remove explicit org.jetbrains.kotlin.android plugin (handled by built-in Kotlin) - Update ProGuard config to use proguard-android-optimize.txt (R8 requirement) - Update managedDevices API to use localDevices (AGP 9.0 change) - Add targetSdk to application defaultConfig Tested with: ./gradlew assembleOssDebug -x detekt ✅ Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Generated by 🚫 Danger Kotlin against f0f03a6 |
- Make rememberFixedTopAppBarState() public with initialStatus parameter to control expanded/collapsed state - Support both scrollable (via rememberTopAppBarState) and non-scrollable (via rememberFixedTopAppBarState) behaviors - State type determines scroll behavior - no need for additional parameters - Add NonScrollableNavBarStory to demonstrate non-scrollable nav bar variants Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Generated by 🚫 Danger Kotlin against afc3cea |
- Add dynamic border width to SearchInputSummary (2.dp when focused, 1.dp otherwise) - Make border transparent for ReadOnly state with isFocused=false - Update BpkTopNavBarTest to use new TopNavBarState parameter for helper functions - Change rememberFixedTopAppBarState default to Collapsed Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Generated by 🚫 Danger Kotlin against 196a4f7 |
- Change BpkModal default navBarState from scrollable to fixed (rememberFixedTopAppBarState) - Add import for rememberFixedTopAppBarState - Modal navigation bar now defaults to non-scrollable behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Generated by 🚫 Danger Kotlin against 20f6bec |
Remember to include the following changes:
README.mdIf you are curious about how we review, please read through the code review guidelines