-
Notifications
You must be signed in to change notification settings - Fork 109
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] Fix trying out Tap to Pay is no-op from the About TTP screen #14340
base: trunk
Are you sure you want to change the base?
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, but I think there might be a better place to make the change. Or, perhaps we have a too-confusing view set up for this flow and should simplify it to make this the right place for the change, I'm not sure.
We present this from two places, the payments menu, and the About TTP screen.
There's a slight difference between the two at the moment; the TapToPaySettingsFlowPresentingView
presented from the menu is wrapped in a NavigationStack
, while the PaymentSettingsFlowPresentingView
presented from the about TTP screen is wrapped in a NavigationView
.
I think we could instead use TapToPaySettingsFlowPresentingView
from both (it's a convenience wrapper for PaymentSettingsFlowPresentingView
) and use NavigationStack
either in the sheets when they're presented, or in the TapToPaySettingsFlowPresentingView
.
Alternatively we might keep it where it is in this PR, but take away some of the extra nav stacks higher up?
All these need a little care to make sure we don't double up the nav bars (which are hidden in some cases), but I think we may have more than we need at the moment...
Up to you but just wanted to point this out so you could check the best way before merging...
N.B. Try a Payment works fine from About when I change the NavigationView
in AboutTapToPayView
's sheet to a NavigationStack
on trunk
, without this PR's changes. However, the onboarding screens it can show before setting up get a nav bar they shouldn't have, but only when shown for the first time on a given store... which is strange. This doesn't happen with the extra stack approach from your PR. So that might be enough reason to just merge as is without digging further in to it!
Closes: #14078
Why
In Payments > About Tap To Pay > Set Up Tap To Pay on iPhone > Try a Payment: in production, nothing happens after the spinner stops as in the "before" screencast below. This is confusing and a broken flow as the merchant cannot test out the payment with Tap To Pay.
How
From debugging, the issue was because
navigationDestination
does not work when the SwiftUI view is not within aNavigationStack
/NavigationView
. In this PR, the top-level view is now wrapped in aNavigationStack
and the navigation works as expected.Steps to reproduce
Testing information
Screenshots
before:
RPReplay_Final1730962609.MP4
after:
RPReplay_Final1730962775.MP4
RELEASE-NOTES.txt
if necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: