Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Shipping Labels Revamp] Introduce hazmat initial navigation and screen #13784

Merged
merged 23 commits into from
Mar 25, 2025

Conversation

ThomazFB
Copy link
Contributor

@ThomazFB ThomazFB commented Mar 17, 2025

Why

Fixes issue #13706. As the Shipping Labels must support the HAZMAT declarations, we should implement a proper flow for the pending Hazardous Materials question standing the main form that's currently a no-op.

How

This PR introduces the initial Hazmat base form with the Composable Screen, ViewModel, and Fragment, while also wires the navigation action of the HAZMAT button in the main creation flow.

⚠️ The complete elements of the form with the clickable links and following navigation action will be introduced in an upcoming PR.

Screen Capture

Screen_recording_20250318_202921.mp4

How to Test

  1. Open an order that allows a Shipping Labels creation
  2. Hit the HAZMAT question in the main creation form
  3. Verify that the Hazmat form opens successfully
  4. Verify that hitting the checkbox for the HAZMAT question enables the Select Category button

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 Mar 17, 2025
This was linked to issues Mar 17, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Mar 17, 2025

📲 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
Commite4e5012
Direct Downloadwoocommerce-wear-prototype-build-pr13784-e4e5012.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Mar 17, 2025

📲 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
Commite4e5012
Direct Downloadwoocommerce-prototype-build-pr13784-e4e5012.apk

@ThomazFB ThomazFB removed a link to an issue Mar 18, 2025
@ThomazFB ThomazFB marked this pull request as ready for review March 18, 2025 23:36
@ThomazFB ThomazFB added this to the 22.0 milestone Mar 18, 2025
@ThomazFB ThomazFB requested review from irfano and atorresveiga March 18, 2025 23:36
import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import androidx.compose.material.Surface

Check warning

Code scanning / Android Lint

material and material3 are separate, incompatible design system libraries Warning

Using a material import while also using the material3 library
@codecov-commenter
Copy link

codecov-commenter commented Mar 18, 2025

Codecov Report

Attention: Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review.

Project coverage is 38.11%. Comparing base (e80e478) to head (e4e5012).

Files with missing lines Patch % Lines
...hippinglabels/WooShippingLabelCreationViewModel.kt 0.00% 3 Missing ⚠️
...bels/hazmat/WooShippingLabelHazmatFormViewModel.kt 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##              trunk   #13784   +/-   ##
=========================================
  Coverage     38.10%   38.11%           
- Complexity     9256     9260    +4     
=========================================
  Files          2079     2080    +1     
  Lines        114560   114579   +19     
  Branches      14593    14593           
=========================================
+ Hits          43657    43672   +15     
- Misses        66936    66940    +4     
  Partials       3967     3967           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@wpmobilebot wpmobilebot modified the milestones: 22.0, 22.1 Mar 21, 2025
@wpmobilebot
Copy link
Collaborator

Version 22.0 has now entered code-freeze, so the milestone of this PR has been updated to 22.1.

@irfano irfano self-assigned this Mar 21, 2025
Copy link
Contributor

@irfano irfano left a comment

Choose a reason for hiding this comment

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

Thanks for dividing this feature into multiple PRs. I’ve added some comments on the code and noticed two issues:

  • The screen isn't readable in dark mode.
  • The screen needs to be scrollable. With the increased font size, some text isn’t readable. This issue will become more noticeable as additional footer text is added.

fun WooShippingLabelHazmatFormScreen(viewModel: WooShippingLabelHazmatFormViewModel) {
val viewState by viewModel.viewState.observeAsState()
WooShippingLabelHazmatFormScreen(
containsHazmatChecked = viewState?.containsHazmatChecked ?: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Equality check can be used instead of elvis for nullable boolean check

The IDE displays this warning, and I agree that using == true is better than perming a null check. Please consider updating it.

viewModel.onSelectCategoryClick()

// Then
assertThat(capturedEvent).isNotNull()
Copy link
Contributor

@irfano irfano Mar 21, 2025

Choose a reason for hiding this comment

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

I think the line checking isNotNull() is redundant since the next line already verifies that capturedEvent is not null.

@ThomazFB ThomazFB force-pushed the issue/introduce-hazmat-initial-navigation branch from 48d9240 to cb73765 Compare March 24, 2025 23:13
@ThomazFB
Copy link
Contributor Author

Hey @irfano! Thanks for your careful review. I addressed all of your suggestions and the PR is ready for a second round.

Copy link
Contributor

@irfano irfano left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, Thomaz. Looks great. 👍🏻

@irfano irfano merged commit 5ed2824 into trunk Mar 25, 2025
17 checks passed
@irfano irfano deleted the issue/introduce-hazmat-initial-navigation branch March 25, 2025 22:21
# for free to join this conversation on GitHub. Already have an account? # 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.

[Hazmat] Initial purchase screen
4 participants