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 - Customs Form] UI 2 #14769

Merged
merged 22 commits into from
Jan 7, 2025

Conversation

toupper
Copy link
Contributor

@toupper toupper commented Dec 27, 2024

Part of: #13784

Description

With this PR we implement the second part of the Shipping Labels Customs forms: the Product Details and the button to save the Custom Details. Please note that we don't implement any data binding here.

Steps to reproduce

  1. Go to orders
  2. Tap on a complete order which can render a shipping label
  3. Tap on Create Shipping Label
  4. Tap on Customs
  5. See that the UI for the Product Details and the bottom button are created

Testing information

N/A

Screenshots

Simulator.Screen.Recording.-.iPhone.16.-.2024-12-27.at.11.25.22.mp4

  • 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 all devices (phone/tablet) and no regressions are added.

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

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 27, 2024

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

App NameWooCommerce iOS WooCommerce iOS
Build Numberpr14769-5d9a810
Version21.3
Bundle IDcom.automattic.alpha.woocommerce
Commit5d9a810
App Center BuildWooCommerce - Prototype Builds #12313
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

Copy link
Contributor

@rachelmcr rachelmcr left a comment

Choose a reason for hiding this comment

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

This is looking good! I noticed some minor issues to address, mostly to match the designs.

VStack {
ScrollViewReader { proxy in
ScrollView {
VStack(alignment: .leading, spacing: Constants.defaultVerticalSpacing) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing an issue with too much vertical spacing between the labels and their corresponding dropdown menu or text field in this VStack, compared to the designs:

Design Actual
Screenshot 2024-12-30 at 16 23 46 Screenshot 2024-12-30 at 16 23 35

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true, I fixed it in 78df820

}
}.renderedIf(isCollapsed)
.foregroundColor(.primary)
.subheadlineStyle()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this text supposed to be subheadline style? The design looks like it's using regular text styling here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Fixed here bee9058

}
.padding(.top, Layout.collapsibleViewTopPadding)
}, content: {
VStack(alignment: .leading, spacing: Layout.collapsibleViewVerticalSpacing) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to above, it looks like there's extra space between the labels and their corresponding text field/dropdown menu in this view, compared to the design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, done in 111bf7b

.foregroundColor(.primary)
.subheadlineStyle()
Spacer()
Image(systemName: "info.circle")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this info circle supposed to be a button?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, done in 71cd530

value: "Description",
comment: "Title for the customs items description text field for customs items")
static let HSTarriffNumber = NSLocalizedString("wooShipping.customsItems.hsTarriffNumber",
value: "HSTarriffNumber",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing its spaces, and I think there's a typo in the design. It should be:

Suggested change
value: "HSTarriffNumber",
value: "HS tariff number",

Copy link
Contributor Author

@toupper toupper Dec 31, 2024

Choose a reason for hiding this comment

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

Done in 6915c9c and f1f3cc6

value: "Optional",
comment: "Placeholder for the HS Tarriff Number text field for customs items")
static let HSTarriffNumberMoreInfo = NSLocalizedString("wooShipping.customsItems.hsTarriffNumber.moreInfoText",
value: "More info about HS tarriff",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a typo for this string in the design:

Suggested change
value: "More info about HS tarriff",
value: "More info about HS tariff",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in f1f3cc6

.footnoteStyle()
}

HStack {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should top-align this HStack so they remain aligned even if one of them is missing a value:

Screenshot 2024-12-30 at 16 38 10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, done in 4ae10d7

Comment on lines 100 to 101
// TODO: Add right unit
Text("kg")
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI the weight unit is set as an environment value, so you could access it with a property like @Environment(\.shippingWeightUnit) var weightUnit: String in this view.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, thanks for the hint, added here 2456a91

.padding(.trailing, Layout.extraPadding)
.padding(.bottom, Layout.extraPadding)
})
.roundedBorder(cornerRadius: Layout.borderCornerRadius, lineColor: Color(.separator), lineWidth: Layout.borderLineWidth)
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the design, the border around the collapsible view could be grey (when collapsed), red (when collapsed if there's an issue with the item), or purple (when expanded).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Done in 1215505


let allCountries: [Country]

let hsTariffURL: URL? = .init(string: "https://woocommerce.com/document/woocommerce-shipping-and-tax/woocommerce-shipping/#section-29")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add this to WooConstants.URLs since it's a known/trusted URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, it looks much better there 00c7eb6

@toupper toupper requested a review from rachelmcr December 31, 2024 11:18
Copy link
Contributor

@rachelmcr rachelmcr left a comment

Choose a reason for hiding this comment

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

🚀

@toupper toupper merged commit a031049 into trunk Jan 7, 2025
13 of 15 checks passed
@toupper toupper deleted the issue/13784-shipping-labels-customs-form-2 branch January 7, 2025 09:13
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.

5 participants