-
Notifications
You must be signed in to change notification settings - Fork 130
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] Introduce Customs initial form #13378
Conversation
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
…duce-customs-section
…duce-customs-section
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #13378 +/- ##
============================================
+ Coverage 41.12% 41.15% +0.03%
- Complexity 6497 6507 +10
============================================
Files 1326 1327 +1
Lines 77601 77661 +60
Branches 10699 10701 +2
============================================
+ Hits 31913 31964 +51
- Misses 42851 42860 +9
Partials 2837 2837 ☔ View full report in Codecov by Sentry. |
Version |
) | ||
} | ||
|
||
is ShowRestrictionTypeDialog -> { |
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.
np: Should we move away from these dialogs and start relying only on Jetpack Compose? Maybe with the use of a Dropdown or a BottomSheet?
This is a np and not a blocker for the PR ☝️
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.
That might be a good idea. I think the best approach would be to adjust this usage on both the Customs and Packages UIs, which are the ones I introduced this old fragment-reliant strategy. Should we add a side nice-to-have task to tackle this improvement as a separate issue? WDYT?
|
||
@Composable | ||
fun WooShippingCustomsFormScreen( | ||
modifier: Modifier = Modifier, |
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.
np: modifier should be the first optional parameter
|
||
fun onITNChanged(newItnValue: String) { | ||
_viewState.update { | ||
it.copy(itnValue = newItnValue) |
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.
From this article, I learned this:
At the moment, it is not recommended to use reactive streams to define the state variable for TextField
That is why I used mutable state for input fields in other screens of the new experience.
That said ☝️ I tested typing new values, and the functionality worked fine, maybe because it is exposed as a livedata
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.
Oh, very interesting approach. Thanks for sharing @atorresveiga! Yeah, I was always curious if the reactive stream approach wouldn't risk to introduce a circular reference of changes, but the Live Data always took care of avoiding that. I agree that we should adopt this strategy.
In that case, similar to what I said in the previous comment, WDYT if we add a separate task to fix this both on the Customs UI and Packages UI? I believe I can actually fix this for the Customs UI in upcoming PRs since I'm still working on that screen, but I think the Packages part also need a bit of attention since it's also using inputs like this.
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 nice and is working as expected. I left a couple of minor notes that are not blockers for this PR.
LGTM!
Thanks for the inputs and review, @atorresveiga! I've replied to your inputs suggesting we to adjust both the TextField reactive approach and adopting the native Compose spinner as fixes for upcoming PRs, since this one is already quite big. So, to unblock the upcoming PRs, I'll merge this one and ensure that both adjustments are covered 🫡 |
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
import androidx.compose.foundation.rememberScrollState | ||
import androidx.compose.foundation.text.KeyboardOptions | ||
import androidx.compose.foundation.verticalScroll | ||
import androidx.compose.material.Button |
Check warning
Code scanning / Android Lint
material and material3 are separate, incompatible design system libraries Warning
import androidx.compose.foundation.text.KeyboardOptions | ||
import androidx.compose.foundation.verticalScroll | ||
import androidx.compose.material.Button | ||
import androidx.compose.material.Checkbox |
Check warning
Code scanning / Android Lint
material and material3 are separate, incompatible design system libraries Warning
import androidx.compose.foundation.verticalScroll | ||
import androidx.compose.material.Button | ||
import androidx.compose.material.Checkbox | ||
import androidx.compose.material.CheckboxDefaults |
Check warning
Code scanning / Android Lint
material and material3 are separate, incompatible design system libraries Warning
import androidx.compose.material.Button | ||
import androidx.compose.material.Checkbox | ||
import androidx.compose.material.CheckboxDefaults | ||
import androidx.compose.material.MaterialTheme |
Check warning
Code scanning / Android Lint
material and material3 are separate, incompatible design system libraries Warning
import androidx.compose.material.Checkbox | ||
import androidx.compose.material.CheckboxDefaults | ||
import androidx.compose.material.MaterialTheme | ||
import androidx.compose.material.Text |
Check warning
Code scanning / Android Lint
material and material3 are separate, incompatible design system libraries Warning
Why
Partially fix issue #13366 by introducing the initial Customs form connected with the main Shipping Labels creation form.
The data validation and error handling will be delivered in the following PR.
The Product Details section will be delivered through issue #13367.
How
Introduces the ViewModel, Screen, and Fragment set for the Customs form, with the navigation action coming from the Shipping Labels creation form. It also defines the basic UI structure alongside the Content, Restriction, and ITN fields, with proper support for the
Other
option, opening an additional Text field for additional description.Unit tests are also added for the ViewModel viewState and events controls.
Screen Capture
Screen_recording_20250130_144547.mp4
How to Test
In case you want to avoid having to fill the address data to access the Customs form, you can simply override the
ShouldRequireCustomsForm
use case always to return true to trigger the Customs data requirement forcefully.Update release notes:
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: