Refactor PayPal Web Demo App into Single View #227
Conversation
|
|
||
| switch selectedFundingSource { | ||
| case .paypalCredit: | ||
| PayPalCreditButton.Representable(color: .black, size: .full) { |
There was a problem hiding this comment.
All the buttons are the same size now. We have a separate Payment Button demo that displays all of the options of size/color/etc.
| .id("bottomView") | ||
| .onAppear { | ||
| withAnimation { | ||
| scrollView.scrollTo("bottomView") |
There was a problem hiding this comment.
Scrolling to the bottom of a view is most commonly used with table views or a view that has incrementing IDs to reference. In order to scroll to the bottom both when displaying this view as well as the bottom of the result after authorizing/capturing we need to duplicate this scrollTo logic. Bit odd but it seemed like the solution for this from what I found online. 🤷♂️
| @@ -6,8 +6,7 @@ class PayPalWebViewModel: ObservableObject, PayPalWebCheckoutDelegate { | |||
|
|
|||
| @Published var state: CurrentState = .idle | |||
| @Published var intent: Intent = .authorize | |||
| @Published var createOrderResult: Order? | |||
| @Published var transactionResult: Order? | |||
| @Published var order: Order? | |||
There was a problem hiding this comment.
We can get rid of the duplicate Order above now that we have a singular result view 🎉
There was a problem hiding this comment.
ux: on Android we do display multiple snapshots of the Order at different points in time. Not sure how it's designed on iOS, but adding some context about why these two values are separate.
There was a problem hiding this comment.
We were mainly using this as a psudo state on iOS to display slightly different results. Since we have 1 result that is appended to as we get more data, we don't need 2 properties.
There was a problem hiding this comment.
Hey @jaxdesmarais , it looks really smooth! I love the toggling of the buttons there in the UI. I am wondering about replacing result view from create with authorize/capture result view. I liked having history of the states, I think that's why we retained the different order snapshots in the first place, to compare the results to make sure that things match up. Another thing that is kind of unexpected for me is that the result view after order creation appears after the toggle for the funding source.
There was a problem hiding this comment.
Just getting back to these comments from being on PTO. 😄
I am wondering about replacing result view from create with authorize/capture result view. I liked having history of the states, I think that's why we retained the different order snapshots in the first place, to compare the results to make sure that things match up.
We can use breakpoints in these cases but should not busy up our demo app with different views containing the same data type. We could also add a quick print statement of the states if needed in each method that returns an order.
Another thing that is kind of unexpected for me is that the result view after order creation appears after the toggle for the funding source.
This made the most sense to keep the result view pinned in the same general place throughout the flow without it jumping around as we added subsequent views. Otherwise, we'd require unnecessary logic to move this view around throughout the flow. I had tested this initially and it was much cleaner to keep the result view relatively in 1 place.
There was a problem hiding this comment.
Hey Jax welcome back! Yeah, I personally like comparing different states instead of new ones swapping out the other for comparison. Steven and I had talked about the one of primary uses for the demo app being able to surface issues or prove that a feature is working correctly. To me, being able to have different states is helpful. It definitely is cleaner the way you refactored it.
To me, being able to see history more clearly without print statements, things appearing chronologically is more important than cleaner code for demo app.
| do { | ||
| let config = try await configManager.getCoreConfig() | ||
| let payPalClient = PayPalWebCheckoutClient(config: config) | ||
| return payPalClient | ||
| } catch { | ||
| updateState(.error(message: error.localizedDescription)) |
There was a problem hiding this comment.
Updated this logic since we wouldn't display anything if we didn't get a config back, no we will return our error view in this case
Demo/Demo/SwiftUIComponents/PayPalWebPayments/PayPalWebViewModel.swift
Outdated
Show resolved
Hide resolved
Demo/Demo/SwiftUIComponents/PayPalWebPayments/PayPalWebViewModel.swift
Outdated
Show resolved
Hide resolved
Reason for changes
Currently the PayPal Web feature exists across 3 total views with totally different result views. This simplifies the PayPal Web feature into a single view. It also gives us the ability to reset the PayPal flow for subsequent checkouts.
Summary of changes
LabelViewTextto stack the text horizontally vs vertically to save spacePayPalWebStatusView- now that we have a single success view we can remove this finePayPalWebButtonsView- this saves space by not displaying all buttons at onceResetbutton toPayPalWebCreateOrderView- this allows for resetting the flow and clearing out stored properties on the ViewModelPayPalWebPaymentsViewOrderStatusPayPalWebStatusViewwithsuccessViewproperty onPayPalWebResultViewPayPalWebTransactionViewnow that we have 1 shared result viewcreateOrderResultandtransactionResult- this can be shared across 1 updatedorderproperty as the order progressesVisuals
Checklist
[ ] Added a changelog entryAuthors