-
Notifications
You must be signed in to change notification settings - Fork 121
[Woo POS][Historical Orders] Order Details (Wireframe UI) #16072
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][Historical Orders] Order Details (Wireframe UI) #16072
Conversation
Generated by 🚫 Danger |
…details-wireframe-ui
82709ef to
eb02d0c
Compare
|
|
fbbef00 to
d4012be
Compare
iamgabrielma
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 well! Looking good 🚢
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.
We got new warnings here, it seems to detect that we had a trailing closure for trailingContent previously, and now is updated with a new bottomContent trailing closure, so it wants to be declared explicitly:
Backward matching of the unlabeled trailing closure is deprecated; label the argument with 'bottomContent' to suppress this warning
|
|
||
| /// Calculates the products subtotal by summing line item subtotals | ||
| /// Follows the same pattern as OrderPaymentDetailsViewModel.subtotal | ||
| func productsSubtotal(for order: POSOrder) -> String { |
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.
Not necessarily as part of this PR (or project), but to avoid repetition on those across features, I wonder if would be worth to move these calculations as functions over POSOrder, rather than keep them as free functions. So rather than helper.productsSubtotal(for: posOrder) we would do posOrder.productsSubtotal() 🤔 WDYT? I'm happy to open some experimentation branch with this if makes sense.
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.
Good idea, I'll see what could be done now 👍
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 ended up following this advice. I found Order+CurrencyFormattedValues, expanded and improved it a bit (fixing some formatting inconsistencies in Woo App order details as well), and then created POSOrdersMapper to set formatted values straight on POSOrder, allowing views to use these values. It simplified the code, reused existing logic, and avoided using CurrencyFormatter (and ServiceLocator) in views.
| struct PointOfSaleOrderDetailsViewHelper { | ||
| private let currencyFormatter: CurrencyFormatter | ||
|
|
||
| init(currencySettings: CurrencySettings = ServiceLocator.currencySettings) { |
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.
Not sure how much this is worth the efford, but should we provide a default value for currencySettings so deep in the view or move it higher? Thinking in the context of getting rid of direct references to ServiceLocator for demodularization
Since PointOfSaleOrderDetailsView() owns PointOfSaleOrderDetailsViewHelper() perhaps is worth to inject it a bit higher. The DI chain is a bit annoying here, so perhaps could sit in PointOfSaleOrderListModel or PointOfSaleOrderListController 🤔
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.
Yes, I agree. I dealt with it during modularization work. What I did was inject the currency settings protocol into the environment so it could be accessed within views. It may be worth bringing some of the modularization helper code to trunk, not to introduce more dependencies...
| /// Formats discount total | ||
| /// Follows TotalsView pattern using discountTotal directly | ||
| func formattedDiscountTotal(for order: POSOrder) -> String? { | ||
| guard let discountTotal = Double(order.discountTotal), discountTotal != 0 else { |
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.
Just double checking since the original implementation checks for >0. Is !=0 what we want here? That would allow for negative discounts depending on how is used, which would be summed to the amount (amount - (-discount))
|
|
||
| /// Formats tax total | ||
| func formattedTaxTotal(for order: POSOrder) -> String? { | ||
| guard let taxTotal = Double(order.totalTax), taxTotal != 0 else { |
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.
Same comment as above regarding >0 vs !=0
WooCommerce/WooCommerceTests/POS/ViewHelpers/PointOfSaleOrderDetailsViewHelperTests.swift
Outdated
Show resolved
Hide resolved
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.
In general, if possible, I'd add when/then to all the test names in the file as well as separate the words that are not defined as variables within the test, ie: singleItem -> single_item
| #expect(result == false) | ||
| } | ||
|
|
||
| @Test func shouldShowDiscount_positiveDiscount_returnsTrue() async throws { |
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.
Let's add a test for negative discounts as well, my understanding is that the negative discount should not be supported, as we detract it from the order amount so its always absolute, but if a negative value happens to go through we may want to handle it here as well.
| ordersViewState = .loading(cachedOrders) | ||
| } | ||
|
|
||
| @MainActor |
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 one does not require MainActor, if we want to run it in the main actor then we need to make the function and conformance async
| @MainActor |
| /// A header view for POS pages. | ||
| /// Design ref: 1qcjzXitBHU7xPnpCOWnNM-fi-450_24951 | ||
| struct POSPageHeaderView<TrailingContent: View>: View { | ||
| struct POSPageHeaderView<TrailingContent: View, BottomContent: View>: View { |
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.
💯
…formatter Injecting currency formatter allows to correctly format totals in the view layer
1c16229 to
f39d665
Compare

WOOMOB-1138
Description
Implement order details UI according to the wireframes. Include a header section with Order ID, Date & Time, Total, Status, Payment Method, and Customer Email. Display the products section and totals section. No actions are included in this task.
POSOrderItemwith additional required fieldsPointOfSaleOrderDetailsViewwith expanded header, products, and totals sectionsPointOfSaleOrderDetailsViewHelperto consolidate formatting and calculations logicOne thing that I didn't include are product images, since they require additional logic I haven't anticipated. I'l added it into a new PR.
Steps to reproduce
Testing information
iPad Air 18.5 Simulator
Screenshots
Loading and navigating orders
Loading.mov
Making a new order & reopening
New.Order.mov
RELEASE-NOTES.txtif necessary.