-
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] Add Starting information screen #13171
Conversation
|
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 found an issue with back navigation from the Preview screen, which shows a deprecated screen, but please feel free to handle that in a separate PR.
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-06-27.at.12.12.04.mp4
@@ -23,6 +24,7 @@ final class AddProductWithAIContainerViewModel: ObservableObject { | |||
|
|||
let siteID: Int64 | |||
let source: AddProductCoordinator.Source | |||
let featureFlagService: FeatureFlagService |
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.
Super nit: is this for use later or redundant? I don't see it being used in this file.
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. I have started using this featureFlagService
now.
private(set) lazy var startingInfoViewModel: ProductCreationAIStartingInfoViewModel = { | ||
.init(siteID: siteID) | ||
}() |
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 migrate away from .init(...)
as a direct init would perform better.
private(set) lazy var startingInfoViewModel: ProductCreationAIStartingInfoViewModel = { | |
.init(siteID: siteID) | |
}() | |
private(set) lazy var startingInfoViewModel = ProductCreationAIStartingInfoViewModel(siteID: siteID) |
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 1874e8b
Image(systemName: Layout.UsePackagePhoto.cameraSFSymbol) | ||
.renderingMode(.template) | ||
.fontWeight(.semibold) | ||
.foregroundColor(Color(.brand)) |
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.
Super nit: I'd suggest using Color.accentColor
just in case we decide to change the accent color or brand color to be different. We should use brand color only for components that are directly related to our branding IMO.
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, Huong! Updated in 11e6e9a
enum Localization { | ||
static let title = NSLocalizedString( | ||
"Starting information", | ||
comment: "Title on the starting info screen." | ||
) | ||
static let subtitle = NSLocalizedString( | ||
"Tell us about your product, what it is and what makes it unique, then let the AI work its magic.", | ||
comment: "Subtitle on the starting info screen." | ||
) | ||
static let placeholder = NSLocalizedString( | ||
"For example: Black cotton t-shirt, soft fabric, durable stitching, unique design", | ||
comment: "Placeholder text on the product features field" | ||
) | ||
static let readTextFromPhoto = NSLocalizedString( | ||
"Read text from product photo", | ||
comment: "Upload package photo button on the text field" | ||
) | ||
static let continueText = NSLocalizedString( | ||
"Continue", | ||
comment: "Continue button on the 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.
Should we follow the latest naming style for localized strings with separate keys and values?
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 1307d5d
Thanks for the review, Huong! 🙇
Good catch. I have fixed this in c88adcc I am enabling auto-merge now. Kindly add comments, if any. |
Part of: #13104
Description
As part of the Product Creation AI v2 work this PR Adds initial version for "Starting information" screen.
Changes
Steps to reproduce
productCreationAIv2M1
feature flag+
button on top rightproductCreationAIv2M1
feature flag and build and run the app.Testing information
Test the product creation with AI flow works as before when
productCreationAIv2M1
feature flag turned off.Screenshots
productCreationAIv2M1
falseproductCreationAIv2M1
trueRELEASE-NOTES.txt
if necessary.