BCDC hook implementation #743
Conversation
🦋 Changeset detectedLatest commit: 183755d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| buyerCountry?: string; | ||
| targetElement?: string | HTMLElement; | ||
| presentationMode?: "auto"; | ||
| fullPageOverlay?: boolean; | ||
| autoRedirect?: boolean; |
There was a problem hiding this comment.
If these are the guest payment presentation mode .start options I think it is fair to do a paypal-js change to consolidate this as a type.
Ideally any change that is in paypal-js should be a separate PR.
| buttonRef?: { current: HTMLElement | null }; | ||
| error: Error | null; | ||
| handleClick: () => Promise<{ redirectURL?: string } | void>; | ||
| handleCancel: () => void; | ||
| handleDestroy: () => void; | ||
| isProcessing?: boolean; |
There was a problem hiding this comment.
If the properties buttonRef and isProcessing are unique to BCDC we could do something like
interface PayPalGuestPaymentReturn extends BasePaymentSessionReturn & {
buttonRef?: { current: HTMLElement | null };
isProcessing?: boolean;
}
and it can be defined in the hook's file 👍
There was a problem hiding this comment.
Right. buttonRef is specific to BCDC. I'll keep it in hook itself.
kand
left a comment
kand
left a comment
There was a problem hiding this comment.
A good start! A few comments now, but I'll continue the review on Monday!
| try { | ||
| await callback(...args); | ||
| } finally { | ||
| resetState(); |
There was a problem hiding this comment.
Under what conditions does this "reset button state"? Reading this, I think this will only reset state if the callback throws an error.
There was a problem hiding this comment.
I was using this "reset button state" to manually reset button after payment flow is completed. We dont need this since SDK is handling the button re-enabling here. https://github.paypal.com/PayPal-R/core-web-sdk/blob/8cc18742de053cf3a8014f13a086d2299526fa96/packages/web-sdk-payments/src/CheckoutSession/PayPalGuestCheckoutSession.ts#L239
| const isSessionActiveRef = useRef(false); | ||
|
|
||
| // Track whether shipping callbacks are present to trigger session recreation | ||
| const hasShippingCallbacks = Boolean( |
There was a problem hiding this comment.
useProxyProps prevents changes to callbacks triggering a component re-render. If I'm understanding hasShippingCallbacks, it appears to be reversing the useProxyProps functionality. Would the same effect be achieved by unpacking the shipping callbacks from the argument on line 39, then having those be dependencies to the session creation useEffect?
There was a problem hiding this comment.
Great point!! That seems to be a much cleaner way to destructure the shipping callbacks from props and passing then to session creation conditionally. Later use them as dependencies to trigger session creation.
| const buttonRef = useRef<HTMLElement>(null); | ||
| const proxyCallbacks = useProxyProps(callbacks); | ||
| const [error, setError] = useError(); | ||
| const [isProcessing, setIsProcessing] = useState(false); |
There was a problem hiding this comment.
I don't see the isProcessing value used anywhere. What would this be used for?
There was a problem hiding this comment.
Initialy I used isProcessing manually disable the button for testing purposes by adding it as a attribute value. Since SDK takes care of disabling the button when the form is open, we no longer need this. Thanks for pointing out. I'll remove it.
| targetElement?: string | HTMLElement; | ||
| presentationMode?: "auto"; | ||
| fullPageOverlay?: boolean; | ||
| autoRedirect?: boolean; |
There was a problem hiding this comment.
BCDC has inline, popup, and modal presentation modes, so any options those can use need to be available. I think fullPageOverlay is valid because it's used by popup. I'm not sure about autoRedirect though, I'm only seeing that in relation to redirect presentation modes. Could you verify if this is an option BCDC needs?
React hook for guest payment session (BCDC).