Repository navigation
Conversation
saurabh-P9525
changed the title
864d8c8 to
a64b89c
Compare
|
Approving. Earlier feedback is addressed and the view composition looks good. Please confirm the SFSafariViewController choice for the Learn more lander (vs ASWebAuthenticationSession) with the team. Add the changelog entry and rebase once 2/6 and 3/6 merge, then /ready. |
Thanks ravi!, I'm keeping |
|
/inner source |
github-actions
Bot
added
inner source
|
/ready |
|
I think you have some build issues going on currently with this PR: |
| struct PayPalCreditMessagingLanderView: UIViewControllerRepresentable { | ||
|
|
||
| let url: URL | ||
|
|
||
| func makeUIViewController(context: Context) -> SFSafariViewController { | ||
| SFSafariViewController(url: url) | ||
| } | ||
|
|
||
| func updateUIViewController(_ controller: SFSafariViewController, context: Context) {} | ||
| } |
There was a problem hiding this comment.
it seems like there's no SFSafariViewControllerDelegate here, so tapping safari's built-in "Done" button never fires safariViewControllerDidFinish(_:) — and without that callback dismissing it, nothing flips viewModel.isLanderPresented back to false. this could potentially result in a case where buyer who taps "Done" could be stuck looking at a lander with no way back short of swiping the sheet down manually.
There was a problem hiding this comment.
I added an SFSafariViewControllerDelegate coordinator, so tapping "Done" now calls back and clears the presented lander in the view model.
| .sheet(isPresented: $viewModel.isLanderPresented) { | ||
| if let url = viewModel.learnMoreURL { | ||
| PayPalCreditMessagingLanderView(url: url) | ||
| } | ||
| } |
There was a problem hiding this comment.
worth adding a check here -> if isLanderPresented ever becomes true while viewModel.learnMoreURL is nil, this presents an empty sheet with nothing in it — there's no else branch or fallback.
There was a problem hiding this comment.
Agreed. The view model now stores the presented lander with its URL, captured at tap time, and the view uses .sheet(item:), so the sheet can't open without a URL.
| /// Read-only binding: the loader is dismissed by the view model, not by user interaction. | ||
| private var editLoaderBinding: Binding<Bool> { | ||
| Binding(get: { viewModel.isEditing }, set: { _ in }) | ||
| } |
There was a problem hiding this comment.
hmm i think the binding's setter is a no-op and it'd be safer to explicitly add .interactiveDismissDisabled(true) on the cover
There was a problem hiding this comment.
Added .interactiveDismissDisabled(true) to the cover. A full-screen cover can't be swiped away anyway, but this makes it explicit that only the view model dismisses it.
| width: EditFIStyleDefaultConstants.editLoaderSpinnerSide, | ||
| height: EditFIStyleDefaultConstants.editLoaderSpinnerSide | ||
| ) | ||
| .foregroundColor(.black) |
There was a problem hiding this comment.
nit: can we use a constant here in place of black like we do for the other params
There was a problem hiding this comment.
Done. Moved it to EditFIStyleDefaultConstants.editLoaderSpinnerColor.
92f0bd9 to
95e4999
Compare
Those errors are because this part depends on parts 2 and 3: at the time it was missing the view model (#1886) and the row components (#1887). #1886 has merged and this branch is rebased on it, and I've confirmed parts 3 and 4 together build and pass all module tests locally. CI here will go green once #1887 merges and I rebase onto it. |
|
Moving back to draft as the build is failing |
|
Approved |
The public view, skeleton row and shimmer, edit loader, Learn more lander and LoadingSpinner asset, with render tests. Rebased onto view-edit-fi-beta-feature after braintree#1887 merged.
…nd gap when hidden, group the view's members, and add a changelog entry
761ccbf to
a6632ef
Compare
| // MARK: - Body | ||
|
|
||
| public var body: some View { | ||
| Group { |
There was a problem hiding this comment.
it looks like the modifier sits on a Group that can resolve to EmptyView. can we verify the loader still dismisses if hasNothingToShow flips mid-edit
There was a problem hiding this comment.
Thanks, I verified this on device simulators (iOS 18.2 and 26.4) by collapsing the component both when the loader is requested and while it's dismissing: the cover still appears and onDismiss fires, so the result is delivered.
| content: { | ||
| EditFlowLoadingView() | ||
| .clearPresentationBackground() | ||
| .interactiveDismissDisabled(true) |
There was a problem hiding this comment.
i think we can remove this, which has no effect on a fullScreenCover
… interactiveDismissDisabled
buzzamus
left a comment
buzzamus
left a comment
There was a problem hiding this comment.
After the most recent changes I believe everything looks good! I just kicked off the integration tests again since I believe this is flakey behavior on a backend service
agedd
left a comment
agedd
left a comment
There was a problem hiding this comment.
thanks for making these changes, looking good 🚀
b87b788
into
braintree:view-edit-fi-beta-feature
8 participants
Part 4 of the View/Edit FI saved payment method component. Adds the public
PayPalSavedPaymentMethodViewthat composes the rows and drives the edit flow.Summary of changes
PayPalSavedPaymentMethodView(public, SwiftUI) takes the checkout request,PayPalSavedPaymentMethodRequest, the client token, the universal link and an optional style, and reports the edit's(nonce, error)throughcompletion.SFSafariViewController(PayPalCreditMessagingLanderView) when PayPal allows embedding, otherwise opens it externally. Safari's "Done" closes it.PayPalSavedPaymentMethodSkeletonRow: the shimmer placeholders shown while loading.Screenshots
Rendered from
PayPalSavedPaymentMethodViewin a test, one per state and a few merchant styles.Component states
Loading
Card + Pay Later
Card, no Pay Later message
PayPal product (label instead of digits)
Email only, editable
Email only, not editable
Brand only (no FI or email)
Merchant style (background, border, corners)
Dark merchant theme
Large accessibility text
Notes for reviewers
AI Usage
Which AI Agent Was Used?
How was AI used?
Used Claude to write the composed view and its render tests, and to split this part out.
Estimated AI Code Contribution
Checklist
Authors
Inner Source Process
Internal to PayPal contributors should fill out this section. All others can delete.
PR should follow these steps before codeowners review will begin:
/inner sourceon this PR — this will automatically add theinner sourceandtech lead review requiredlabels. Open the PR in a draft state./readyon this PR — this will automatically remove thetech lead review requiredlabel. Move the PR to ready to review.Inner Source Checklist