-
Notifications
You must be signed in to change notification settings - Fork 121
[Mobile Payments] Add badges leading to Set up Tap to Pay #9801
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] Add badges leading to Set up Tap to Pay #9801
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. LGTM! 🚢
|
|
||
| /// Represents a regular UITableView Cell: [Image | Text | Disclosure] | ||
| /// | ||
| class BadgedLeftImageTableViewCell: UITableViewCell { |
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.
Just wondering: Wouldn't inherit like class BadgedLeftImageTableViewCell: LeftImageTableViewCell remove the need to declare leftImage, labelText, ... and we could have only the extensions to setup this view?
| extension NSNotification.Name { | ||
| /// Posted whenever the hub menu view did appear. | ||
| /// | ||
| public static let setUpTapToPayViewDidAppear = Foundation.Notification.Name(rawValue: "com.woocommerce.ios.setUpTapToPayViewDidAppear") |
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.
That's convenient! 💯
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.
Yep. Notifications need to be used with care, but I think it's appropriate here.
| .scrollVerticallyIfNeeded() | ||
| } | ||
| .padding() | ||
| .onAppear(perform: viewModel.viewDidAppear) |
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.
Just to clarify if I understood correctly why this method call: This is basically a configuration method, right? We're not adding it to the viewmodel's init because we don't want to rely on the viewModel's lifecycle, and wait for this to be initialized to trigger this configuration. And at the same time, we want the config logic to be testable, so we extract it from the view to the model.
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.
IIRC, we're not using the init because the view model is created optimistically, the user may not actually open the screen. We use this viewModel.viewDidAppear to trigger the notification, which we respond to in other places by removing all the badges... so we only want to do that when it actually appears.
| viewModel.$shouldBadgeTapToPayOnIPhone.sink { [weak self] _ in | ||
| self?.configureSections() | ||
| // ensures that the cell will be configured with the correct value for the badge | ||
| DispatchQueue.main.async { | ||
| self?.tableView.reloadData() | ||
| } | ||
| }.store(in: &cancellables) |
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 don't think is necessary as we're already weakifying self, but should we add a guard just to remove optionality?
| viewModel.$shouldBadgeTapToPayOnIPhone.sink { [weak self] _ in | |
| self?.configureSections() | |
| // ensures that the cell will be configured with the correct value for the badge | |
| DispatchQueue.main.async { | |
| self?.tableView.reloadData() | |
| } | |
| }.store(in: &cancellables) | |
| viewModel.$shouldBadgeTapToPayOnIPhone.sink { [weak self] _ in | |
| guard let self = self else { return } | |
| self.configureSections() | |
| DispatchQueue.main.async { | |
| self.tableView.reloadData() | |
| } | |
| }.store(in: &cancellables) | |
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 don't think there's a benefit to removing the optionality here, but thanks for the suggestion 😊
Part of: #9582
Description
To inform merchants about Tap to Pay being available, we are adding badges to lead merchants to the Set up Tap to Pay on iPhone screen.
These will be shown to merchants whose phone and site could support Tap to Pay, but who have not used it yet.
The badges are added to:
Menutab itemPaymentsrow in the Hub MenuSet up Tap to Pay on iPhonerow in the Payments MenuAll badges will remain until the merchant opens the
Set up Tap to Pay on iPhonescreen – after this, they will be removed, regardless of whether the merchant goes through the flow. Alternatively, if they take a Tap to Pay payment without going through the Set up flow, the badges will also be removed.This PR adds the badges, and some of the removal logic, but is missing three key bits of logic
These items will be done in a future PR. This work is feature flagged by
featureFlagService.isFeatureFlagEnabled(.tapToPayBadge)Testing instructions
Open the app
Observe that the badge is shown on the Menu tab
Navigate to
MenuObserve that the badge is shown on the Payments row
Navigate to
PaymentsObserve that the badge is shown on the
Set up Tap to Pay on iPhonerowTap
Set up Tap to Pay on iPhoneTap
CancelObserve that the badges are removed, on the
Menutab and theSet up Tap to Pay on iPhonerowTap
<Observe that the badge is removed on the
Paymentsrow.Screenshots
badge-set-up-tap-to-pay.mp4
(n.b. I fixed the slight delay in badging the row when the In Person Payments Menu is loaded after recording this video, in 6c81716)
RELEASE-NOTES.txtif necessary.