Move POS custom amounts entry to a row in the products list#17013
Move POS custom amounts entry to a row in the products list#17013toupper wants to merge 4 commits intorsm/pos-custom-amounts-order-detailsfrom
Conversation
Generated by 🚫 Danger |
|
|
8cb949e to
67dbf4b
Compare
Replaces the cart-header `+` icon (and its TipKit popover) with a `CustomAmountEntryRow` rendered above the products list when: - the products tab is selected, - `pointOfSaleCustomAmounts` is on, - the order stage is `.building`, - the merchant isn't currently searching. The row is product-card-shaped — same dimensions and visual rhythm as `SimpleProductCardView` — with a tag icon and a "Tap to add a one-off charge" subtitle, so it reads as part of the "things I can add" list the merchant already scans. This trades the low-discoverability cart icon for a glanceable affordance and lets us drop the TipKit popover that existed solely to teach the cart icon. State plumbing - The form's presentation state (`isCustomAmountSheetPresented`, `editingCustomAmount`) moves from `CartView` to `PointOfSaleAggregateModel`, with `presentAddCustomAmount`, `presentEditCustomAmount`, and `dismissCustomAmountSheet` methods. This lets entry points anywhere in the view hierarchy drive the same sheet without prop drilling. - `CartView` keeps the `posFullScreenCover` attachment, reading state via `@Bindable` on the aggregate model. - The cart row's pencil button now calls `posModel.presentEditCustomAmount(_:)` directly. Removed - `CartAddCustomAmountButton` (the cart-header `+` icon) - `AddCustomAmountTip` and `Tips.configure` in `PointOfSaleEntryPointView` - `CartViewHelper.shouldShowAddCustomAmountButton(...)` and its three tests - `import TipKit` from `CartView` and `PointOfSaleEntryPointView` - `import CocoaLumberjackSwift` from `PointOfSaleEntryPointView` (was only used for the TipKit configure error log) Follow-ups - The empty-cart state still says "Tap on a product to add it to the cart, or [Scan barcode]" — that copy stays accurate since custom amounts are now reachable from the products list. No empty-state change needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Extract the entry row's gating logic into a new `ItemListViewHelper.shouldShowCustomAmountEntryRow` and add five Swift Testing cases covering the tab, feature flag, order stage, and searching axes (mirrors how `CartViewHelper` was unit-tested). - Drop the restating block comment on the now one-line wrapper; the doc comment lives on the helper next to the rule. - Set explicit `accessibilityLabel` and `accessibilityHint` on the entry row (with `.accessibilityElement(children: .ignore)`) so VoiceOver reads "Custom amount" with "Tap to add a one-off charge" as a hint, instead of concatenating the two into one label. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Switch the row's icon from `tag` to `tag.badge.plus` so the affordance reads as "add a new fee" rather than "category". - Drop the imperative "Tap to" prefix; the row's tap target speaks for itself. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fbb8601 to
ca2b18f
Compare
`tag.badge.plus` either doesn't exist as an SF Symbol or renders inconsistently across iOS versions; the row showed up blank in testing. Replace it with `tag` + a `plus.circle.fill` overlay in the bottom-trailing corner, which renders correctly on every supported iOS release and conveys the "add a custom amount" affordance unambiguously. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| .padding(.horizontal, Constants.horizontalTextPadding * (1 / scale)) | ||
| .padding(.vertical, Constants.verticalTextPadding * (1 / scale)) | ||
| Spacer() | ||
| } |
There was a problem hiding this comment.
.accessibilityElement(children: .ignore) collapses the subtree into a single synthesized element, which drops the Button's .isButton trait. VoiceOver will read the label and hint but won't announce it as a button. Could you add the trait explicitly?
.accessibilityElement(children: .ignore)
.accessibilityLabel(Text(Localization.title))
.accessibilityHint(Text(Localization.subtitle))
.accessibilityAddTraits(.isButton)
.accessibilityIdentifier("pos-custom-amount-entry-row")| static let title = NSLocalizedString( | ||
| "pos.itemList.customAmountEntryRow.title", | ||
| value: "Custom amount", | ||
| comment: "Title for the row in the Point of Sale products list that opens the custom amount form.") |
There was a problem hiding this comment.
POS guidelines ask for an iPad-sized preview (the module is iPad-first) and ideally a Dynamic Type variant. The current single phone-sized preview is fine for a sanity check but doesn't show the layout under the conditions this row will actually run in. Worth adding a second preview at an accessibility size, since the row branches on dynamicTypeSize.isAccessibilitySize.
| } | ||
|
|
||
| /// Whether the custom amount entry sheet is currently presented. | ||
| var isCustomAmountSheetPresented: Bool = false |
There was a problem hiding this comment.
isCustomAmountSheetPresented is a fully settable var so CartView can @Bindable-bind it to posFullScreenCover. SwiftUI will write false directly through that binding on swipe-to-dismiss, and AddCustomAmountView.submit() also writes isPresented = false — both paths skip dismissCustomAmountSheet(), which is the only place that nils editingCustomAmount.
No live bug today (the next presentAddCustomAmount() resets it), but it's a stale-state foot-gun — anything that reads editingCustomAmount between dismiss and the next opener will see the previous edit target.
Two easy fixes:
- Make
isCustomAmountSheetPresentedprivate(set)and expose aBinding<Bool>whose setter routesfalsethroughdismissCustomAmountSheet(). - Or attach
onDismiss: { posModel.dismissCustomAmountSheet() }to theposFullScreenCoverand have the form calldismissCustomAmountSheet()instead of writingisPresenteddirectly.
| cart.removeCustomAmount(id: id) | ||
| } | ||
|
|
||
| func presentAddCustomAmount() { |
There was a problem hiding this comment.
The neighboring mutating methods (addMoreToCart, startNewCart, setStateForEditing, …) are all @MainActor, but these three new ones aren't. Probably harmless in practice since the callers are SwiftUI views, but worth annotating for consistency and to keep non-UI callers honest:
@MainActor func presentAddCustomAmount() { … }
@MainActor func presentEditCustomAmount(_ customAmount: POSCustomAmount) { … }
@MainActor func dismissCustomAmountSheet() { … }| func dismissCustomAmountSheet() { | ||
| isCustomAmountSheetPresented = false | ||
| editingCustomAmount = nil | ||
| } |
There was a problem hiding this comment.
While you're in PointOfSaleAggregateModel, would be great to lock down the new state with a few quick @Test cases (no existing PointOfSaleAggregateModelTests so it'd be a new file):
presentAddCustomAmount→isCustomAmountSheetPresented == true,editingCustomAmount == nilpresentEditCustomAmount(x)→isCustomAmountSheetPresented == true,editingCustomAmount == xdismissCustomAmountSheetafter edit → both cleared
The third one would also catch the cleanup-bypass concern above if/when it gets fixed.
| POSBarcodeScannerSetup(isPresented: $showBarcodeScanningModal, analytics: analytics) | ||
| } | ||
| .posFullScreenCover(isPresented: $showCustomAmountSheet) { | ||
| .posFullScreenCover(isPresented: $posModel.isCustomAmountSheetPresented) { |
There was a problem hiding this comment.
One structural thing to think about: the sheet's posFullScreenCover is attached to CartView's body, but CartView is conditionally rendered in PointOfSaleDashboardView (hidden during full-screen card payment / cash success). The new entry point lives in ItemListView. Today it's safe because shouldShowCustomAmountEntryRow already requires orderStage == .building, but the trigger and the sheet host are now in two different views and the dependency isn't visible from either side.
Consider lifting the posFullScreenCover up to PointOfSaleDashboardView, which is where the always-rendered settings cover already lives — would match the spirit of "any view can drive the sheet."
| struct ItemListViewHelperTests { | ||
| let sut = ItemListViewHelper() | ||
|
|
||
| @Test func shouldShowCustomAmountEntryRow_when_all_conditions_met_then_true() { |
There was a problem hiding this comment.
Small convention thing: testing.md and Modules/Tests/CLAUDE.md ask for // Given / // When / // Then comment blocks, and the rest of the POS suite (e.g. CartViewHelperTests) sticks to it. Could you add them here? The branches you've covered are great, just want the structure to match.
samiuelson
left a comment
There was a problem hiding this comment.
LGTM. Tests good on iPad. I shared code improvement suggestions from claude.

Description
Replaces the cart-header
+icon (and its TipKit popover) with aCustomAmountEntryRowrendered above the products list. When the merchant is in the products tab, building an order, and not currently searching, the first row in the list reads "Custom amount — Tap to add a one-off charge" with a tag icon. Tapping it opens the existing full-screen form.This trades the low-discoverability cart icon for a glanceable affordance that lives where merchants already scan ("things I can add to the order"). Discussed both this and the literal variations-style left-pane push; we picked this approach because:
posFullScreenCoverfor the form itself.ItemsStackState/POSItemto model "form-as-sub-state" — keeps the items model focused on listings.State plumbing
The form's presentation state (
isCustomAmountSheetPresented,editingCustomAmount) moves fromCartViewtoPointOfSaleAggregateModel. Any entry point can callpresentAddCustomAmount,presentEditCustomAmount(_:), ordismissCustomAmountSheet, driving the same sheet without prop-drilling.CartViewkeeps theposFullScreenCoverattachment, reading state via@Bindable.Removed
CartAddCustomAmountButton(the cart-header+icon)AddCustomAmountTipandTips.configuresetupCartViewHelper.shouldShowAddCustomAmountButton(...)and its three testsimport TipKit,import CocoaLumberjackSwiftreferences that only existed to support the popoverThis PR stacks on top of #16999. It auto-rebases onto trunk as the rest of the stack lands.
Test Steps
On a
localDeveloperoralphabuild (custom amounts flag on):+icon.On an App Store build (flag off):
+, no TipKit popover.Screenshots
See the feature walkthrough video and design rationale in the feature announcement post.