-
Notifications
You must be signed in to change notification settings - Fork 120
Shipping Labels: Update tabs for shipments with purchased labels #15463
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
Conversation
|
|
…s (temporary solution)
| let shipmentID = container.failsafeDecodeIfPresent(targetType: String.self, | ||
| forKey: .shipmentID, | ||
| alternativeTypes: [.integer(transform: { String($0) })]) |
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.
While testing again I saw that the shipment ID was somehow returned as a number today. This is a workaround to transform the number to our expected string type.
selanthiraiyan
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.
The code looks great and the given test cases all passed. ![]()
For an order with products with more than one quantities (with sub items), I noticed that after the shipments are saved from mobile the web isn't able to properly parse the saved information. i.e. The split shipments interface in web isn't working properly after the shipment is split from the mobile app.
SplitShipmentsInWeb.mov
This could be either an issue with how we save the shipments or how the web parses the shipments info. I think we should log a separate issue for this and investigate it.
I have logged an issue here. #15469
| if viewModel.currentShipment.isPurchased { | ||
| VStack { | ||
| Text(Localization.PurchasedShipment.title) | ||
| .bold() | ||
| .frame(maxWidth: .infinity, alignment: .leading) | ||
| Text(Localization.PurchasedShipment.subtitle) | ||
| .frame(maxWidth: .infinity, alignment: .leading) | ||
| } | ||
| .multilineTextAlignment(.leading) | ||
| .foregroundStyle(Layout.green) | ||
| .padding(Layout.contentPadding) | ||
| .background( | ||
| Layout.greenBackground | ||
| .clipShape(RoundedRectangle(cornerRadius: Layout.cornerRadius)) | ||
| ) | ||
| } |
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.
super-nit: Consider moving this into a computed property to keep the main body less crowded. Perhaps named as shipmentPurchasedMessageView.
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.
Done in 4e310d2.
| let totalOrderItems = order.items.map(\.quantity).reduce(0, +) | ||
| if totalOrderItems > 1 { | ||
| // Only fetch shipments info if there are more than one order items. | ||
| group.addTask { | ||
| await self.loadShipmentsInfo() | ||
| } |
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.
Suggestion: We could move this totalOrderItems check logic into loadShipmentsInfo to keep the shipments-related logic inside one method.
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 plan to remove this check when working on #15309, so I'll keep this for now.
| let purchasedIcon = UIImage(systemName: "checkmark.circle.fill")?.withRenderingMode(.alwaysTemplate) | ||
| return TopTabItem(name: String.localizedStringWithFormat(Localization.shipmentFormat, index + 1), | ||
| icon: item.isPurchased ? purchasedIcon : nil, | ||
| content: { EmptyView() }) |
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.
nit: We could initialise the icon outside of the map to avoid creating the image for every shipment.
| let purchasedIcon = UIImage(systemName: "checkmark.circle.fill")?.withRenderingMode(.alwaysTemplate) | |
| return TopTabItem(name: String.localizedStringWithFormat(Localization.shipmentFormat, index + 1), | |
| icon: item.isPurchased ? purchasedIcon : nil, | |
| content: { EmptyView() }) | |
| let purchasedIcon = UIImage(systemName: "checkmark.circle.fill")?.withRenderingMode(.alwaysTemplate) | |
| return shipments.enumerated().map { (index, item) in | |
| TopTabItem(name: String.localizedStringWithFormat(Localization.shipmentFormat, index + 1), | |
| icon: item.isPurchased ? purchasedIcon : nil, | |
| content: { EmptyView() }) | |
| } |
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.
Done in 8a33bbb.
|
Thank you @selanthiraiyan for the review!
Good catch! I have addressed the suggestions in subsequent commits, enabling auto-merge for now. |

Closes: #15440
Description
This PR implements a few changes to support showing existing shipments on the split shipments screen:
Note: the design found for the purchased shipment shows the checkbox disabled, but I think hiding the checkbox is simpler so I'm keeping that behavior.
Steps to reproduce
Testing the Create shipping label button on the order details screen
Testing the split shipments entry point
Testing existing shipments
Testing information
Tested and confirmed the above test cases in simulator iPhone 16 Pro iOS 18.2.
Screenshots
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-04-01.at.18.13.15.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: