-
Notifications
You must be signed in to change notification settings - Fork 121
Product Preview: Add "Preview" button to product details #7960
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
Conversation
# Conflicts: # Experiments/Experiments/DefaultFeatureFlagService.swift # Experiments/Experiments/FeatureFlag.swift
|
Reverted unit tests on |
You can test the changes from this Pull Request by:
|
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 looks good, my main worry is one suggestion to make sure all cases are covered and tested!
Also I think we need to update some test cases.
I went through the test cases and have a couple of concerns/questions
- If you change a published product status to draft (using the products settings screen) you get the preview button, should that be the case? or should we wait until the product is actually saved as a draft before showing the preview button?
change-product.mov
- If you start creating a new product, you get immediately the preview button. Is that ok or should we wait for at least one change?
| case .productsPreview: | ||
| return buildConfig == .localDeveloper || buildConfig == .alpha |
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 think we could use the same .productsOnboarding feature flag right?
When the other deliverables are done, we can just take them out of the feature flag check.
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.
Right, updated in edf3702
| updatedQueryItems.append(.init(name: "preview", value: "true")) | ||
| permalink?.queryItems = updatedQueryItems | ||
| guard let url = permalink?.url else { | ||
| return |
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.
Can this fail? if it can, should we show some kind of toast?
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 source string, product.permalink is non-optional property. But all URL manipulation tools in iOS SDK return optional URL result.
So I consider this one is safe and never reachable.
| if ServiceLocator.featureFlagService.isFeatureFlagEnabled(.productsPreview), | ||
| (canSaveAsDraft() || productModel.status == .draft) { | ||
| buttons.insert(.preview, at: 0) | ||
| } |
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.
Do you think we could move this logic to the switch above, or would things get more complicated? The switch is useful for us to not miss any case or combination.
Also, do you think this is worth unit testing?
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 @Ecarrion for useful feedback and suggestions!
Agree! I've added test initially, but rolled them back because of feature flag conflict.
I agree that we shouldn't allow preview here. Because it will trigger "unpublishing" of a product before user actually tap "save" to confirm a change.
I don't see much issue in that, but also there is no value :) |
# Conflicts: # Experiments/Experiments/FeatureFlag.swift # WooCommerce/WooCommerceTests/Mocks/MockFeatureFlagService.swift
Ecarrion
left a comment
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.
Looks great! Thanks for the changes! 🚀
Closes: #7944
Description
This PR adds button on product details to show a preview. Button should appear:
It triggers draft saving (if needed) before opening webview with preview URL (unauthenticated at this stage).
The whole new feature is hidden behind new feature flag.
Testing
Screenshots
RELEASE-NOTES.txtif necessary.