[POS Orders] Move sales channel representation to Networking #15863
Conversation
iamgabrielma
added
type: task
|
|
|
|
||
| // Then | ||
| XCTAssertEqual(viewModel.salesChannel, OrderListCellViewModel.Localization.salesChannelPOSText) | ||
| XCTAssertEqual(viewModel.salesChannel, "POS") |
There was a problem hiding this comment.
I had to revert the localization here as now the localized string sits in Networking 🤔
There was a problem hiding this comment.
I think this is fine, a bunch of unit tests are already based on the English locale assumption.
jaclync
left a comment
jaclync
left a comment
There was a problem hiding this comment.
Thanks for making the updates! LGTM 
Unrelated to this PR, the POS badge seems to be truncated in large font size in both the order list and order details. We can also wait for the final design to make any accessibility enhancements.
| large font size | small font size |
|---|---|
 |
 |
| @@ -0,0 +1,32 @@ | |||
| import Foundation | |||
|
|
|||
| public enum SalesChannelType { | |||
There was a problem hiding this comment.
nit: how about just SalesChannel? Doesn't seem like there is a naming conflict, just wondering.
There was a problem hiding this comment.
No special reason, updated here! 98aaff9
| public var description: String { | ||
| switch self { | ||
| case .pointOfSale: | ||
| return NSLocalizedString("saleschanneltype.pos.description", |
There was a problem hiding this comment.
nit: or salesChannel.pos.description depending on the final name of the enum
| return NSLocalizedString("saleschanneltype.pos.description", | |
| return NSLocalizedString("salesChannelType.pos.description", |
| init(order: Order, | ||
| status: OrderStatus?, | ||
| calendar: Calendar = .current) { | ||
| self.order = order |
There was a problem hiding this comment.
nit: I see a salesChannel: String? above, maybe can set either the sales channel description or enum to the salesChannel property here?
There was a problem hiding this comment.
True 🤔 , I've updated it on 7df616b but since the enum is in Networking I've continued using a String, in which case we can remove also some duplication around the description
|
|
||
| // Then | ||
| XCTAssertEqual(viewModel.salesChannel, OrderListCellViewModel.Localization.salesChannelPOSText) | ||
| XCTAssertEqual(viewModel.salesChannel, "POS") |
There was a problem hiding this comment.
I think this is fine, a bunch of unit tests are already based on the English locale assumption.
|
Thanks for the review!
Good catch, specially the order list details seem to break very easily in larger accessibility sizes, I don't think we'll get much additional design feedback around these badges apart from the recently updated colors, so I'll take a look into this next 👍 |
iamgabrielma
deleted the
task/move-sales-channel-representation-to-networking
branch
Description
As follow-up from p1751433227323889-slack-CGPNUU63E , we move the sales channel representation from WooCommerce to Networking, removing some code duplication, and avoiding having to handle the
pos-rest-apiraw value directly in the app layer.Let me know if this is what you had in mind @jaclync , happy to iterate further!
Changes
SalesChannelType, where thecreated_viavalue is passed directly and returns the specific associated case and it's description ( for now, only.pointOfSale-"POS"is handled, any other value will return nil)Testing information