Adding a package UI - Add package name text field #14086
Conversation
bozidarsevo
added
the
feature: shipping labels
Generated by 🚫 Danger |
|
|
…caffolding-package-name
…caffolding-package-name
|
Version |
…caffolding-package-name
| @@ -1,5 +1,22 @@ | |||
| import SwiftUI | |||
|
|
|||
| final class WooShippingAddPackageViewModel: ObservableObject { | |||
There was a problem hiding this comment.
Could we add unit tests for this view model? Not sure if you have that planned as a separate task.
Also, a small thing, but we usually add the view model as a separate file. Not a huge deal at this stage but it will likely help as we expand it to include more logic e.g. network calls.
| // Holds type of selected package, it can be `custom`, `carrier` or `saved` | ||
| @State var selectedPackageType = PackageProviderType.custom | ||
| var addPackageButtonDisabled: Bool { | ||
| for (_, value) in fieldValues { | ||
| if value.isEmpty { | ||
| return true | ||
| } | ||
| } | ||
| return fieldValues.count != WooShippingAddPackageDimensionView.DimensionType.allCases.count | ||
| } | ||
| // Holds selected package type when custom package is selected, it can be `box` or `envelope` | ||
| @State var packageType: PackageType = .box | ||
| // Holds value for toggle that determines if we are showing button for saving the template | ||
| @State var showSaveTemplate: Bool = false | ||
| @State var packageTemplateName: String = "" |
There was a problem hiding this comment.
What do you think about moving these to the view model, as well? It could be helpful for unit testing, and for handling the values e.g. for packageType and packageTemplateName in network calls.
There was a problem hiding this comment.
I was planning to but left it here in case I change something, but will move it! 👍
| Text(Localization.saveNewPackageTemplate) | ||
| .font(.subheadline) | ||
| } | ||
| .tint(Color(.withColorStudio(.wooCommercePurple, shade: .shade60))) |
There was a problem hiding this comment.
I'd suggest using the semantic color Color(.primary) or Color(.accent) here (and elsewhere in this view). That makes it easier to adjust later if we update the app colors, and it adjusts the color in dark mode to fit our usual dark mode theme.
| // MARK: - actions | ||
|
|
||
| private func validateCustomPackageInputFields() -> Bool { | ||
| guard !customPackageViewModel.areFieldValuesInvalid else { | ||
| return false | ||
| } | ||
| if showSaveTemplate { | ||
| return !packageTemplateName.isEmpty | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| private func addPackageButtonTapped() { | ||
| // TODO: implement adding a package | ||
| guard validateCustomPackageInputFields() else { return } | ||
| } | ||
|
|
||
| private func savePackageAsTemplateButtonTapped() { | ||
| // TODO: implement saving package as a template | ||
| guard validateCustomPackageInputFields() else { return } |
There was a problem hiding this comment.
I'd suggest putting actions like these in the view model, so we can unit test them.
| .onChange(of: showSaveTemplate) { newValue in | ||
| packageTemplateNameFieldFocused = newValue |
There was a problem hiding this comment.
Do you think we should also reset packageTemplateName to an empty string when this is toggled? It feels a little odd that any text entered there is retained if I toggle this off and on again.
There was a problem hiding this comment.
I can reset it but I am thinking what would be the least bad case:
- user enters a name, accidentally toggles it off, toggles it back on and the text is cleared (user needs to enter text again)
- user enters a name, toggles it off on purpose, toggles it back on and sees the last entered text (user can save with that name or change it) - my preference
I would prefer that the string is kept until the template is saved, and then we reset it. But we can see when we test it with more people what feels better?
There was a problem hiding this comment.
Yeah, good point, that's a fair concern.
But we can see when we test it with more people what feels better?
Sounds good!
| })) | ||
| .onChange(of: packageTemplateNameFieldFocused) { focused in | ||
| if focused { | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + Constants.scrollToDelay, execute: { |
There was a problem hiding this comment.
What's the reasoning behind the delay for this scroll action? It feels fine in testing, I'm just curious if it's needed for some reason or if it's just for the feel.
There was a problem hiding this comment.
When text field for name gets focused (packageTemplateNameFieldFocused changed) the SwiftUI automatically scrolls so that text field is visible, but the save button is not, and if I do not do the scrolling to button with a small delay this is basically called in paralel as scrolling to text field so save button is overlayed a bit with keyboard.
This is how it goes without a delay (half of the button is visible):
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-10-18.at.09.45.17.mp4
There was a problem hiding this comment.
Ah yeah, that makes sense. Thanks for the explanation! Maybe it's worth a small note in the code about that so that later on we remember why it's needed?
…oShippingAddPackageViewModelTests
|
Version |
…caffolding-package-name
…caffolding-package-name
There was a problem hiding this comment.
Looks great and continues to test well. 👍 I have a few minor questions below, mainly about the unit tests, but nothing blocking.
Also this is something to deal with later, maybe after checking with design, but I noticed that the screen isn't really usable in landscape mode. Just sharing it now since I just saw this:
| let viewModel = WooShippingAddCustomPackageViewModel() | ||
|
|
||
| // When | ||
| viewModel.clearFieldValues() |
There was a problem hiding this comment.
Given that it inits with empty field values, how about entering some values (e.g. the viewModel.fillWithDummyFieldValues() method you use below) before clearing them in this test?
| let viewModel = WooShippingAddCustomPackageViewModel() | ||
|
|
||
| // When | ||
| viewModel.clearFieldValues() |
There was a problem hiding this comment.
Do you need this step in the test? If it inits with empty fields I'd think we could skip this and just do:
// When
viewModel.fieldValues[.height] = "1"
| let viewModel = WooShippingAddCustomPackageViewModel() | ||
|
|
||
| // When | ||
| viewModel.clearFieldValues() |
There was a problem hiding this comment.
Same as above, do you need this step in these tests?
| func resetValues() { | ||
| clearFieldValues() | ||
| showSaveTemplate = false | ||
| packageTemplateName = "" | ||
| } |
There was a problem hiding this comment.
Should this also reset the packageType to .box? I could see it going either way (remembering the previously used package type or resetting it), but just asking because I noticed that if I select the envelope type that isn't reset.
Part of #13551
Description
Features remaining for the "Custom" tab:
Features remaining for the "Carrier" tab:
Features remaining for the "Saved" tab:
Rest of the functionalities will be done in separate stacked PRs (done on top of this branch or trunk if this gets merged before).
Testing information
revampedShippingLabelCreationfeature flag enabled.Screenshots
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-10-16.at.11.26.10.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: