Skip to content
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

[Shipping Labels Revamp] Update Carrier and Saved packages data for better state control #13104

Merged
merged 17 commits into from
Dec 12, 2024

Conversation

ThomazFB
Copy link
Contributor

@ThomazFB ThomazFB commented Dec 10, 2024

Summary

Partially fix issue #12747 by introducing a series of adjustments to the Carrier and Saved packages data to allow a complete state control with loading and error handling.

The upcoming PR will be focused on triggering the selected package to the main Shipping Labels UI.

Screenshots

Screen_recording_20241211_194209.mp4

How to Test

  1. Go to the Order Details of an Order eligible for Shipping Label Creation
  2. Hit the Create Shipping Labels button
  3. Once inside the new Shipping Labels form, hit the Select a Package button
  4. Verify that the Carrier and Saved tabs display their loading feedback properly
  5. Verify that the Add Package button is only enabled for the tab with an actual selected Package
  6. Verify that the Custom Package form logic is working as expected

Update release notes:

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if 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:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

@ThomazFB ThomazFB added type: task An internally driven task. feature: shipping labels Related to creating, ordering, or printing shipping labels. labels Dec 10, 2024
@ThomazFB ThomazFB added this to the 21.3 milestone Dec 10, 2024
@dangermattic
Copy link
Collaborator

dangermattic commented Dec 10, 2024

1 Warning
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 10, 2024

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit16a8e5b
Direct Downloadwoocommerce-wear-prototype-build-pr13104-16a8e5b.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 10, 2024

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit16a8e5b
Direct Downloadwoocommerce-prototype-build-pr13104-16a8e5b.apk

@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 26.47059% with 75 lines in your changes missing coverage. Please review.

Project coverage is 40.47%. Comparing base (0b3861d) to head (16a8e5b).
Report is 214 commits behind head on trunk.

Files with missing lines Patch % Lines
...abels/packages/components/PackageItemComponents.kt 0.00% 49 Missing ⚠️
...ckages/WooShippingLabelPackageCreationViewModel.kt 39.47% 16 Missing and 7 partials ⚠️
...i/orders/wooshippinglabels/packages/ui/UIModels.kt 72.72% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk   #13104      +/-   ##
============================================
- Coverage     40.51%   40.47%   -0.04%     
  Complexity     6190     6190              
============================================
  Files          1287     1287              
  Lines         74229    74286      +57     
  Branches      10169    10192      +23     
============================================
- Hits          30074    30069       -5     
- Misses        41544    41598      +54     
- Partials       2611     2619       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ThomazFB ThomazFB linked an issue Dec 11, 2024 that may be closed by this pull request
@ThomazFB ThomazFB changed the title [Shipping Labels Revamp] Split PackageData dimensions into length, width and height [Shipping Labels Revamp] Update Carrier and Saved packages data for better state control Dec 11, 2024
@ThomazFB ThomazFB marked this pull request as ready for review December 11, 2024 22:45
@atorresveiga
Copy link
Contributor

Good work @ThomazFB ! I checked that:

✅ The Carrier and Saved tabs display their loading feedback properly
✅ The Add Package button is only enabled for the tab with an actual selected Package
✅ The Custom Package form logic is working as expected

Some notes from my testing not related to the changes in this PR:

  • Seems that vertical scroll is missing on some of the screens. I tried to scroll down on the Carrier screen with no luck and look at this weird behaviour when creating a custom package
Screen_recording_20241211_230523.mp4

Adding a custom package works, but we need to add some feedback (loading state)

Copy link
Contributor

@atorresveiga atorresveiga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! :shipit:

import org.assertj.core.api.Assertions.assertThat
import kotlin.test.Test

class PackageDataTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ThomazFB
Copy link
Contributor Author

Hey @atorresveiga! Thank you so much for testing 🙇 Regarding the scroll, I actually fixed that yesterday but noticed that the fix went with the other PR 🤦
image

To avoid doing git cherry picks or complicating things, I'll leave this to be fixed in that PR since the adjustment is already there, WDYT? 😬

Adding a custom package works, but we need to add some feedback (loading state)

I agree with your point! There's one thing, though. When I left the Custom package creation without loading feedback, the reasoning was how we could avoid making the user wait for the package to be saved. We could be "optimistic" about the request and just move forward with the Shipping Labels form.

With that, when we hit the Add Package button, the data will be sent immediately to both the Shipping Labels form and the networking layer, making the experience feel snap and removing the need for the loading feedback. WDYT?

@ThomazFB
Copy link
Contributor Author

@atorresveiga I'll move forward with this PR merge to unblock the next one, and depending on what we decide from the discussion here, I can introduce some fixes through the next PR. Hope that's ok!

@ThomazFB ThomazFB merged commit 774e71c into trunk Dec 12, 2024
24 of 28 checks passed
@ThomazFB ThomazFB deleted the issue/add-split-package-data-dimensions branch December 12, 2024 15:30
@atorresveiga
Copy link
Contributor

With that, when we hit the Add Package button, the data will be sent immediately to both the Shipping Labels form and the networking layer, making the experience feel snap and removing the need for the loading feedback. WDYT?

My only concern is what will happen if the save request fails? If we move to the shipping label form, we cannot recover from that, and the package will not be saved. WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: shipping labels Related to creating, ordering, or printing shipping labels. type: task An internally driven task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Shipping Labels: Packages] Add Package Opening UI Section
5 participants