Skip to content

[WASABI-11828] Update BpkPrice - #2845

Open
Fernando Piardi (fpiardi) wants to merge 8 commits into
mainfrom
wasabi/WASABI-11828_UpdateBpkPrice
Open

Fernando Piardi (fpiardi) wants to merge 8 commits into
mainfrom
wasabi/WASABI-11828_UpdateBpkPrice

Conversation

@fpiardi

@fpiardi Fernando Piardi (fpiardi) commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Extends BpkPrice (Compose) to support an optional icon-decorated, tappable leadingText, and fixes two pre-existing text-alignment bugs for BpkPriceAlign.End.

Figma: https://www.figma.com/design/0DqzOBrdMfjyf0wST86WrG/Price-Pulse?node-id=2451-12998&p=f&t=dDlA1obi3YLqpItX-0

New API (BpkPrice.kt)
Added three new optional parameters:

  • leadingIcon: BpkIcon? — icon rendered before leadingText
  • trailingIcon: BpkIcon? — icon rendered after leadingText
  • onLeadingTextClicked: (() -> Unit)? — when set, leadingText + its icons become a single tappable, accessible target (merged semantics)
    All are no-ops when leadingText is null. No changes to existing params/behavior when the new ones aren't used.

New internal component

  • internal/BpkPriceLeadingText.kt — renders leadingIcon + leadingText + trailingIcon as one row, optionally clickable (clickableWithRipple + semantics(mergeDescendants = true)), icon tint matches style.secondaryTextColor() so it stays correct for both default and onContrast.

Bug fixes

  1. BpkPriceAlignEnd.kt — price/trailingText not right-aligned. Neither the price Text nor trailingText had textAlign set, so both always rendered left-aligned within their own box — not just a multi-line-wrap edge case, but any time the component had more available width than the text needed (the common case in real layouts). Fixed by setting textAlign = TextAlign.End on both.
  2. Confirmed via Compose TextMeasurer/semantics-bounds inspection that this is because Paragraph always reports width = maxWidth constraint (not content width) when bounded, regardless of alignment — so without an explicit textAlign, glyphs defaulted to flush-left inside that box.

Known limitation (documented, not fixed — by design decision)

  • BpkPriceAlignStart.kt: trailingText shares a Row with the (possibly multi-line) price. If the price wraps and its longest line fills the available width, trailingText is left with zero width and collapses invisibly. BpkPriceAlignEnd doesn't have this issue since it stacks trailingText on its own line below the price. Left as-is per discussion; revisit if it becomes a real-world problem.

Demo (PriceStory.kt)

  • Added within the existing Default/OnContrast stories (not a new story category):
  • 6 leadingIcon/trailingIcon combination examples (leading-only / trailing-only / both, for Start and End)
  • Long, digits-only price example (£1,830,000,000,000,000) demonstrating multi-line wrapping, with Start and End side by side
  • Made the story scrollable (verticalScroll) so all examples remain reachable on-device.

Docs (docs/compose/Price/README.md)

  • Added usage snippets for leadingIcon/trailingIcon/onLeadingTextClicked and for the long/wrapping price case.

Testing notes

  • BpkPriceTest (existing Roborazzi screenshots) — the End-align fixes are real, intentional visual corrections (not noise); confirmed via pixel-diff and TextMeasurer analysis that most End goldens with price/trailingText content will need re-recording when this lands.

Remember to include the following changes:

  • Component README.md
  • Tests

If you are curious about how we review, please read through the code review guidelines

Copilot AI balanced review requested due to automatic review settings October 1, 2026 13:04
@fpiardi Fernando Piardi (fpiardi) added minor A new & backwards compatible feature/component ai: claude labels Oct 1, 2026
@skyscanner-backpack-bot

Copy link
Copy Markdown
Contributor
Warnings
⚠️ One or more component files were updated, but the tests weren't updated. If your change is not covered by existing tests please add snapshot tests.
⚠️

One or more component files were updated, but the docs screenshots weren't updated. If the changes are visual or it is a new component please regenerate the screenshots via ./gradlew recordScreenshots.

⚠️

One or more package files were created, but BpkComposeComponentUsageDetector.kt wasn't updated. If your component is an equivalent of a core component please add it to the detector.

Generated by 🚫 Danger Kotlin against eb78acb

@skyscanner-backpack-bot

Copy link
Copy Markdown
Contributor
Warnings
⚠️ One or more component files were updated, but the tests weren't updated. If your change is not covered by existing tests please add snapshot tests.
⚠️

One or more component files were updated, but the docs screenshots weren't updated. If the changes are visual or it is a new component please regenerate the screenshots via ./gradlew recordScreenshots.

⚠️

One or more package files were created, but BpkComposeComponentUsageDetector.kt wasn't updated. If your component is an equivalent of a core component please add it to the detector.

Generated by 🚫 Danger Kotlin against 2c7a9b6

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: None

What changed in this PR

This PR extends the Compose BpkPrice component to support optional icons around leadingText and an optional click target for the leading text area, plus improves alignment for wrapped, end-aligned prices. It also updates the docs and demo to showcase the new API and multi-line wrapping behavior.

Changes:

  • Add leadingIcon, trailingIcon, and onLeadingTextClicked support to BpkPrice (and internal implementations).
  • Introduce BpkPriceLeadingText to render leading text with optional icons and a single accessible click target.
  • Improve multi-line alignment for end-aligned price/trailing text and update docs/demo examples.
File Description
docs/​compose/​Price/​README.md Adds examples for clickable leading text/icons and long wrapping prices
backpack-compose/​src/​main/​kotlin/​net/​skyscanner/​backpack/​compose/​price/​internal/​BpkPriceRow.kt Adds leading icon/click support and refactors leading text rendering
backpack-compose/​src/​main/​kotlin/​net/​skyscanner/​backpack/​compose/​price/​internal/​BpkPriceLeadingText.kt New internal composable for leading text + icons with unified click target
backpack-compose/​src/​main/​kotlin/​net/​skyscanner/​backpack/​compose/​price/​internal/​BpkPriceLabel.kt Adds optional textAlign to improve wrapping alignment for non-clickable prices
backpack-compose/​src/​main/​kotlin/​net/​skyscanner/​backpack/​compose/​price/​internal/​BpkPriceImpl.kt Wires new leading icon/click params through to alignment implementations
backpack-compose/​src/​main/​kotlin/​net/​skyscanner/​backpack/​compose/​price/​internal/​BpkPriceAlignStart.kt Adds leading icon/click support in start alignment variant
backpack-compose/​src/​main/​kotlin/​net/​skyscanner/​backpack/​compose/​price/​internal/​BpkPriceAlignEnd.kt Adds leading icon/click support and applies end textAlign for wrapped lines
backpack-compose/​src/​main/​kotlin/​net/​skyscanner/​backpack/​compose/​price/​BpkPrice.kt Public API extension: new params for leading icons and leading-text click
app/​src/​main/​res/​values/​strings.xml Adds demo strings for new examples (cheaper label, long price)
app/​src/​main/​java/​net/​skyscanner/​backpack/​demo/​compose/​PriceStory.kt Adds new demo scenarios and makes the story scrollable

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@skyscanner-backpack-bot

Copy link
Copy Markdown
Contributor
Warnings
⚠️

One or more component files were updated, but the docs screenshots weren't updated. If the changes are visual or it is a new component please regenerate the screenshots via ./gradlew recordScreenshots.

⚠️

One or more package files were created, but BpkComposeComponentUsageDetector.kt wasn't updated. If your component is an equivalent of a core component please add it to the detector.

Generated by 🚫 Danger Kotlin against a7a9a55

if (onClick != null) {
base
.clickable(
role = Role.Button,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should use Modifier.clickableWithRipple(...) for consistency.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai: claude minor A new & backwards compatible feature/component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants