-
Notifications
You must be signed in to change notification settings - Fork 120
Shipping Labels: Show tabs in creation form #15483
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
…mentDetailsViewModel
Generated by 🚫 Danger |
|
|
|
Version |
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.
Great effort here! Thanks a lot for driving this feature ahead of Android, @itsmeichigo .
I reviewed the code but didn't find any specific feedback to share. (Excuse my iOS skills!) Below are my notes from testing:
-
When I entered the split shipments screen, only the “Select All” option and a disabled “Done” button were visible. I couldn’t navigate back, but I managed to do so using the ESC key on my keyboard. I haven't seen the "Done" button in a disabled state in the Figma designs, so I believe it should always be enabled.
-
As you can see in the screenshots below, the bottom sheets display the total number of products, while the shipment cost section displays the cost of the selected shipment. I think the other part of the bottom sheet should also reflect information specific to the selected shipment.
-
We should disable modifying the purchased shipment.
-
I opened the split shipment screen, switched tabs, and navigated back using the ESC key. After that, I encountered the inconsistency shown below.
|
Thanks for the ping @JorgeMucientes! I checked the code and it looks good to me, so I will approve it, but it would idea to check @irfano feedback above 👍 |
toupper
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.
LGTM!
|
Thank you @toupper for checking the code! And thank you @irfano for doing the manual tests and sharing great feedback 🙇
Previously, we enabled the Done button, but this would trigger a remote update for the shipments. We decided to only enable it when there are actual changes, but we're still waiting for design input on how to replace the Select All button with Cancel to enable dismissal (more on the thread here p1743465824357759-slack-C03L1NF1EA3). For now, I'll wait for Wagner's response before updating the buttons in a separate PR.
I was confused about this when working on this PR too. I checked with the web and saw that on the side bar, the order items are not changed when switching between shipments. I suppose they consider this panel the details of the order instead of shipments. What do you think about this?
If you mean the shipment tab doesn't show the purchased label, that's a known issue as stated in the PR description and should be handled separately for #15487.
Navigating back using ESC key is a hack on the simulator, and I don't think there's an equivalent to that on a physical device. I'll note down this issue to check after updating the buttons on the splitting shipments screen as mentioned above. For the rest of the issues, they were not changed in this PR, so I'll create a separate issue for updates on the split shipment screen. Please feel free to leave any additional comments about my response here. For now, I'll merge this PR and will add improvements in subsequent PRs. |
Hmm, okay. I checked the web version too. When switching shipments, the "Order details" always show general order information, while the “Shipment details” section updates according to the selected shipment. That matches the behavior in this PR, so let’s keep it as is. I take back my earlier feedback.
I’m not sure if it’s related, but this is what I meant: On this screen, Shipment 1 is already purchased, but I can still move an item from Shipment 2 to Shipment 1, which shouldn’t be allowed.
|
Good catch! I'll log a separate issue for this. |







Closes: #15309
Description
This PR updates the logic on the shipping labels creation form to support showing shipments in tabs. Changes include:
Notes:
Steps to reproduce
Testing information
Confirm the test cases above with simulator iPhone 16 Pro iOS 18.2.
Screenshots
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-04-04.at.17.09.32.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: