-
Notifications
You must be signed in to change notification settings - Fork 114
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
[Product Creation AI v2] Select package image #13193
Conversation
📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.
|
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.
This works as expected 👍 I left some nit-picking comments below.
viewModel.startingInfoViewModel.onPickPackagePhoto = { [weak self] source in | ||
await self?.presentImageSelectionFlow(source: source) | ||
} |
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.
❓Could you explain why need to use async here? It seems to me that we're mixing the logic of presenting the selection flow with the selected image.
Would it be possible to separate the logic like what we did previously with the old flow? i.e. We keep the presentation of the image picker flow synchronous; and when an image is selected, we trigger a method in startingInfoViewModel
with the selected image. Would that work?
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.
Could you explain why need to use async here? It seems to me that we're mixing the logic of presenting the selection flow with the selected image.
We are using async
because we will send the image view into a loading state and wait for the image to be received from this method. Code here
Would it be possible to separate the logic like what we did previously with the old flow? i.e. We keep the presentation of the image picker flow synchronous; and when an image is selected, we trigger a method in startingInfoViewModel with the selected image. Would that work?
I actually followed the pattern from AddProductFromImageCoordinator
and made this method async
. We use the same pattern in BlazeEditAdHostingController
as well. Please correct me if I am misunderstanding things.
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'd prefer a more naive implementation for separation of concerns. I'm also worried about the use of async
because it could potentially come with complications when migrating to Swift 6. Please use your own judgement to decide whether to rewrite 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.
Thanks for the input, Huong!
After considering this, I prefer to keep the implementation as is.
My thoughts are,
- Separation of concerns wise,
SelectPackageImageCoordinator
takes care of selecting and downloading the image.SelectPackageImageCoordinator
also encapsulates the different paths of camera capture, device media library and WP Media library image selection. After initial selection, all those paths require async operations to prepare theMediaPickerImage
. i.e. We present the selection flow and wait to get back aMediaPickerImage
. - Using
await
inProductCreationAIStartingInfoViewModel
makes it easy to maintain the previous image state and display a loading indicator until the image is ready. - We can alter this if we find any complications while migrating to Swift 6.
SelectPackageImageCoordinator
along withBlazeEditAdHostingController
(We will removeAddProductFromImageCoordinator
while removing package flow code)
Please let me know if you still think we need to change this. I will add a subtask and work on this at the end of the project. 🙏
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'm good with keeping this as-is for now, thanks for the explanation!
enum Localization { | ||
static let photoUploaded = NSLocalizedString( | ||
"productCreationAIStartingInfoView.packagePhotoView.viewPhoto", | ||
value: "Photo uploaded", |
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: Should we use "Photo selected" instead? "uploaded" sounds like the photo has been uploaded somewhere - it's misleading to me.
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.
Good point. Done in a995988
static let photoUploaded = NSLocalizedString( | ||
"productCreationAIStartingInfoView.packagePhotoView.viewPhoto", | ||
value: "Photo uploaded", | ||
comment: "Button title in starting info screen." |
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: we should explain clearly what the text is about to make it easier for translators. Same comment for the below strings.
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 aba6505
Part of: #13104
Description
Adds a way to select a package photo for the product creation with AI flow.
Changes
Menu
to view, replace and remove selected package photo.Steps to reproduce
productCreationAIv2M1
feature flag+
button on top rightScreenshots
PackageSelection.mp4
RELEASE-NOTES.txt
if necessary.