-
Notifications
You must be signed in to change notification settings - Fork 108
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
Products: Grouping design for product creation type sheet #12273
Conversation
📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.
|
…t's not likely to be reused.
…pes in a category. This can happen if, for example, a store doesn't support subscription products creation.
5ce53b1
to
65acb1d
Compare
@@ -77,13 +76,13 @@ public enum BottomSheetProductType: Hashable, Identifiable { | |||
case .simple(let isVirtual): | |||
if isVirtual { | |||
return NSLocalizedString( | |||
"bottomSheetProductType.simpleVirtual.title", | |||
value: "Simple virtual product", | |||
"bottomSheetProductType.simpleVirtualProduct.title", |
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.
Have to also change the key for the string value change to apply. Not completely sure why, but without doing this, the string change doesn't appear in the simulator.
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 update looks good! I wonder if we should also replace the bottom sheet for the case where AI is not eligible to use the same sheet but without the AI option. Currently, when testing for such sites, I still see the old sheet. Please feel free to file an issue for this as an enhancement separately though.
I left a few suggestions for code improvements below, but I'm pre-approving this PR.
.padding(.bottom, Constants.categoryVerticalSpacing) | ||
.padding(.horizontal, Constants.horizontalSpacing) | ||
|
||
ForEach(productTypes, id: \.self) { productType in |
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.
Nit: BottomSheetProductType
is already conforming to Identifiable
, so we can omit the id
parameter here.
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.
👍🏼 updated in 34442d1
Text(category.label) | ||
.subheadlineStyle() | ||
.textCase(.uppercase) |
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.
TIL about the .textCase
modifier. Normally I would simply use Text(category.label.uppercased)
to transform the string directly.
ForEach(productTypes, id: \.self) { productType in | ||
HStack(alignment: .top, spacing: Constants.margin) { | ||
Image(uiImage: productType.actionSheetImage.withRenderingMode(.alwaysTemplate)) | ||
.font(.title3) |
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.
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.
Updated in 34442d1
} | ||
} | ||
} | ||
} | ||
|
||
private extension ManualProductTypeOptions { | ||
enum Constants { | ||
// List of product categories. The ordering dictates how the categories are displayed. | ||
static let productCategoriesOrder: [BottomSheetProductType.ProductCreationCategory] = [.standard, .subscription, .other] |
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.
Suggestion: You can omit this if you make BottomSheetProductType.ProductCreationCategory
conform to CaseIterable
. Then you can get the list of cases in the order declared in the enum with BottomSheetProductType.ProductCreationCategory.allCases
.
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.
Done in f1b37ed also this comment is related to this #12273 (comment)
var productCreationCategory: ProductCreationCategory { | ||
switch self { | ||
case .simple, .blank, .variable: | ||
return .standard | ||
case .subscription, .variableSubscription: | ||
return .subscription | ||
case .custom, .grouped, .affiliate: | ||
return .other | ||
} | ||
} |
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.
Suggestion: you can avoid having to filter and match the category types if you replace this with a helper variable in ProductCreationCategory
instead:
extension ProductCreationCategory {
var productTypes: BottomSheetProductType {
// return the respective types for each category.
}
}
Then in ManualProductTypeOptions
you can get rid of groupedProductTypes
and build the view by directly traversing the categories:
ForEach(BottomSheetProductType.ProductCreationCategory.allCases, id: \.self) { category in
ForEach(category.productTypes) { type in
// TODO
}
}
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.
I did some refactor on f1b37ed based on your ideas here, but it doesn't fully implement your suggestion as I understood it.
The main thing is that the ManualProductTypeOptions
receives supportedProductTypes
that contains only the supported product types (calculated in presentActionSheetWithAI()
in AddProductCoordinator
), so filtering things based on it is inevitable.
@hafizrahman Is this PR being actively worked on? Just checking because it has been moved to a future milestone a handful of times already. (I'll go ahead and move it to the next milestone for now.) |
@rachelmcr this was a hack week project that I didn't end up having enough time to complete, then got deprioritized over other tasks. Thanks for the reminder, I'll set as draft for now and come back to it next hack week. |
@hafizrahman It would be great to wrap this PR for this HACK week as well, it has been around for quite a while. |
Closes: #11361, #11211
Description
This PR is the second part of the product type creation bottom sheet.
Specifically, it now groups the "Manual product creation" into three categories:
Testing instructions
Video
Simulator.Screen.Recording.-.iPhone.15.-.2024-03-14.at.19.46.45.mp4
RELEASE-NOTES.txt
if necessary.