-
Notifications
You must be signed in to change notification settings - Fork 121
[Mobile Payments] Badge tap to pay on eligible devices and stores, until Set up Tap to Pay opened #9812
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
[Mobile Payments] Badge tap to pay on eligible devices and stores, until Set up Tap to Pay opened #9812
Conversation
You can test the changes from this Pull Request by:
|
iamgabrielma
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.
Tests well!
✅ Using an iPhone XS or newer on iOS 16 or above, and a US-based store with WCPay or the Stripe extension
✅ Using an iPhone XS or newer on iOS 16 or above, and a US-based store with WCPay or the Stripe extension
✅ Using an iPhone X or older, or an iPad, or any device running iOS 15, and a US-based store with WCPay or the Stripe extension
✅ Using an iPhone XS or newer on iOS 16 or above, and a CA or GB-based store with WCPay
LGTM! Just shared a couple of comments that can be tackled separately, if needed.
| observePlanName() | ||
| listenToNewFeatureBadgeReloadRequired() | ||
| retrieveShouldShowNewFeatureBadgeOnPaymentsValue() | ||
| tapToPayBadgePromotionChecker.$shouldShowTapToPayBadges.share().assign(to: &$shouldShowNewFeatureBadgeOnPayments) |
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 wrap this line in a method? Sort of "bind/observe badge changes"?
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.
Yeah, it is a bit wordy. I'll add this to a list to do when I have a little time.
|
|
||
| listenToNewFeatureBadgeReloadRequired() | ||
| retrieveShouldShowNewFeatureBadgeOnHubMenuTabValue() | ||
| tapToPayBadgePromotionChecker.$shouldShowTapToPayBadges.share().assign(to: &$shouldShowNewFeatureBadgeOnHubMenuTab) |
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.
Same as HubMenuViewModel: Should we wrap this line in a method? Sort of "bind/observe badge changes"?
|
|
||
| private var cancellables = Set<AnyCancellable>() | ||
|
|
||
| let tapToPayBadgePromotionChecker: TapToPayBadgePromotionChecker = TapToPayBadgePromotionChecker() |
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.
Perhaps for another PR: Should we abstract this class into some PromotionChecker protocol to keep it detached from the specific TTP promotion checker? It could make sense also inject the protocol in the initializer for testability and to use this common protocol in the future for other feature announcements.
|
|
||
| private let willPresentReviewDetailsFromPushNotification: () async -> Void | ||
|
|
||
| private let tapToPayBadgePromotionChecker: TapToPayBadgePromotionChecker |
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.
Same comment as with MainTabViewModel regarding abstracting this class into a protocol, as would seem that we're leaking implementation details about the badges into the HubMenu navigation coordinator responsibility.
Closes: #9582
Description
N.B. this follows #9801, but changes the logic and display details substantially. If you'd prefer to review both together, feel free to change the base of this PR to
trunk.We've added badges leading to Set up Tap to Pay on iPhone, for eligible devices and stores. This PR resolves the three outstanding logic issues from #9801:
Now the badges will only be shown when you are using a supported device, an eligible store, have not previously used Tap to Pay to take a transaction, and will remember when the badges have been dismissed.
Testing instructions
Using an iPhone XS or newer on iOS 16 or above, and a US-based store with WCPay or the Stripe extension
Menutab, and thePaymentsrow in the menu, and theSet up Tap to Pay on iPhonerow in the IPP menuSet up Tap to Pay on iPhone, then tap backUsing an iPhone XS or newer on iOS 16 or above, and a US-based store with WCPay or the Stripe extension
Menutab, and thePaymentsrow in the menu, and theSet up Tap to Pay on iPhonerow in the IPP menuCollect Paymentand collect a payment using Tap to Pay on iPhoneUsing an iPhone X or older, or an iPad, or any device running iOS 15, and a US-based store with WCPay or the Stripe extension
Menutab, and thePaymentsrow in the menu, and theSet up Tap to Pay on iPhonerow in the IPP menuUsing an iPhone XS or newer on iOS 16 or above, and a CA or GB-based store with WCPay
Menutab, and thePaymentsrow in the menu, and theSet up Tap to Pay on iPhonerow in the IPP menuScreenshots
RELEASE-NOTES.txtif necessary.