-
Notifications
You must be signed in to change notification settings - Fork 120
[Woo POS] Coupons: Products/Coupons Header #15512
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Woo POS] Coupons: Products/Coupons Header #15512
Conversation
- Accept multiple items with titles, subtitles, and action - Change selected item index on action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the POS header view to support switching between Products and Coupons by introducing a multi-item header and an associated round action button. Key changes include:
- Adding a new POSPageHeaderItem structure and updating POSPageHeaderView to render multiple header items with selection support.
- Creating a new POSPageHeaderActionButton view for displaying a round action button.
- Adjusting ItemListView to use the new header view and control the display logic based on feature flags.
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| WooCommerce/Classes/POS/Presentation/Reusable Views/POSPageHeaderView.swift | Added multi-item support with selectable header buttons and updated accessibility traits. |
| WooCommerce/Classes/POS/Presentation/Reusable Views/POSPageHeaderActionButton.swift | Created a new view for rendering a round action button. |
| WooCommerce/Classes/POS/Presentation/Reusable Views/POSHeaderLayoutConstants.swift | Added a new constant (minHeight) for header layout consistency. |
| WooCommerce/Classes/POS/Presentation/ItemListView.swift | Updated header rendering logic to integrate multiple header items and apply conditional rendering for the info button. |
Files not reviewed (1)
- WooCommerce/WooCommerce.xcodeproj/project.pbxproj: Language not supported
Comments suppressed due to low confidence (1)
WooCommerce/Classes/POS/Presentation/Reusable Views/POSPageHeaderView.swift:74
- [nitpick] Consider using items.enumerated() or iterating directly over items (e.g. ForEach(items)) to leverage the Identifiable conformance of POSPageHeaderItem, which may improve code clarity and reduce reliance on index arithmetic.
ForEach(0..<items.count, id: \ .self) { index in
| .padding(Constants.infoIconInset) | ||
| }) | ||
| .renderedIf(!shouldShowHeaderBanner) | ||
| .renderedIf(!shouldShowHeaderBanner && !shouldShowCoupons) |
Copilot
AI
Apr 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review the combined condition for rendering the info button to ensure that the button is hidden only when both header banners and coupons are active, as this logic change affects the UI behavior.
| .renderedIf(!shouldShowHeaderBanner && !shouldShowCoupons) | |
| .renderedIf(!(shouldShowHeaderBanner && shouldShowCoupons)) |
|
|
joshheald
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as expected and matches designs.
It gets pretty funky when the search field is shown at any dynamic type size; you get an animation effect on the titles changing size, as they're smaller when they're constrained by the search field, but then grow when you switch to coupons. That's not a blocker because you won't be able to switch tabs in search mode.
You can see a subtler version of the same issue when at very large dynamic sizes and constrained width, but it's acceptable for the extreme text size used. It's caused by the + button only being shown on Coupons.
Zoom.effect.on.title.mp4
| var items = [ | ||
| POSPageHeaderItem( | ||
| title: Localization.productsTitle, | ||
| action: { | ||
| displayItemListType(.products(search: searchTerm.isNotEmpty)) | ||
| } | ||
| ) | ||
| ] | ||
|
|
||
| if shouldShowCoupons { | ||
| items.append( | ||
| POSPageHeaderItem( | ||
| title: Localization.couponsTitle, | ||
| action: { | ||
| displayItemListType(.coupons) | ||
| } | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice and clean at the callsite
| .font(.posButtonSymbolSmall) | ||
| .foregroundColor(.posOnSurface) | ||
| } | ||
| .frame(width: POSHeaderLayoutConstants.minHeight, height: POSHeaderLayoutConstants.minHeight) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This starts to look a little odd at larger dynamic type sizes. Perhaps use a @ScaledMetric inited with the min height, rather than the raw value? You can use the min of that and the constant, to avoid the button getting too small.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried playing with ScaledMetric, likely min and max values are required since it also grows a bit too gigantic. I'll try to combine them.
|
|
||
| private var hStackAlignment: VerticalAlignment { | ||
| subtitle == nil ? .center: .firstTextBaseline | ||
| items.first?.subtitle == nil ? .center: .firstTextBaseline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the subtitle going to be used for something soon, or just future proofing? I can't see where it is at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshheald, thanks, good observations. We could reserve some space for action buttons by keeping them invisible rather than removing them, then the amount of space for headers wouldn't change. Either way, this header is not great for extreme dynamic type sizes. Adding a third header would be enough to break it or it would require a different layout. If I look at native Apple apps, they do limit the dynamic type growth in quite a few places. Tabs at the bottom and top grow quite little when even the default font is much smaller than ours. We need to do user testing, but I think large elements should grow relatively less, given they are already large. |
…tabs-switch-header-designer
Hide the button with opacity rather than remove from hierarchy so the space would always be reserved for this button. It keeps the header titles size consistent when switching between the products and coupons while having large dynamic type sizes.
Constraint between original and 1.5x times the size for the best UX
I think that titles/tabs which are part of a small, consistent are also less likely to be read in detail. Error message text, for example, is much more important to fully scale as it'll happen rarely and the user needs to know what's going on. |


Closes: #
Description
Creating a header view for switching between Products and Coupons
POSPageHeaderViewto support multiple titles with actionPOSPageHeaderActionButtonfor displaying a round action buttonItemListViewto display a new header if the feature flag is enabledInfo button is deliberately removed based on the designs and discussions during the design phase.
Steps to reproduce
Enable
enableCouponsInPointOfSalefeature flagTesting information
Tested on iPad Air 11 Inch iOS 18.4 Simulator
Screenshots
Header View
products-coupons-switch.mp4
Hidden Simple Products Banner
simple.and.variable.products.mp4
RELEASE-NOTES.txtif necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: