-
Notifications
You must be signed in to change notification settings - Fork 663
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
[connect] Update SDK API then Example app as needed #10330
Conversation
Diffuse output:
APK
|
@@ -115,7 +111,6 @@ fun ComponentPickerContent( | |||
activity = context, | |||
title = "Account Onboarding", | |||
props = onboardingSettings.toProps(), | |||
cacheKey = "AccountOnboardingExample" |
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.
👀 cacheKey
is unnecessary for Controllers since there can only be one.
actions = { | ||
IconButton( | ||
onClick = { | ||
onSave( |
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.
👀 Removed Save button in favor of save-on-change, since the UX is better with the toast.
cacheKey: String? = null, | ||
) : StripeComponentController<AccountOnboardingListener> by StripeComponentControllerImpl( | ||
cls = AccountOnboardingDialogFragment::class.java, | ||
) : StripeComponentController<AccountOnboardingListener, AccountOnboardingProps>( | ||
dfClass = AccountOnboardingDialogFragment::class.java, |
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.
👀
- As mentioned above,
cacheKey
is not necessary for Controllers since there can only be one DialogFragment per component type. StripeComponentController
was converted to an abstract class with its implementation was migrated fromStripeComponentControllerImpl
@@ -89,7 +86,7 @@ class EmbeddedComponentManager( | |||
* @param cacheKey Key to use for caching the internal WebView across configuration changes. | |||
*/ | |||
@PrivateBetaConnectSDK | |||
fun createAccountOnboardingView( | |||
internal fun createAccountOnboardingView( |
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.
👀 API change
|
||
/** | ||
* Controller for a full screen component. | ||
*/ | ||
@PrivateBetaConnectSDK | ||
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) | ||
interface StripeComponentController<Listener : StripeEmbeddedComponentListener> { | ||
abstract class StripeComponentController<Listener, Props> internal constructor( |
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.
👀 As mentioned earlier, this was changed to an abstract class with an internal constructor. Its implementation was merged in from StripeComponentControllerImpl
, which was deleted. There are no logic changes.
/** | ||
* A full-screen DialogFragment that displays a full-screen component. | ||
* | ||
* The implementation to make this all work *well* is quite tricky and deserves some explanation. |
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.
👀 These implementation notes are from StripeComponentControllerImpl
. I didn't put this in StripeComponentController
due to previous feedback to reduce its visibility to users.
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.
Can you please split this up into multiple PRs? This makes review so much easier and is also much better for commit and PR history
I'm thinking that since there are 4 bullet points of things that happened here, this should definitely be multiple PRs. And the API changes could at least be their own PR with a link to the updated API review so I can see some context on the changes.
Thank you!!
Will do, but I will flag that with recent CI flakiness this could potentially be really painful. |
Summary
Update the Connect SDK API and Example app following API review. Some API behavior was removed, so the Example app UX needed to be updated as appropriate:
The diff is big, but it's mostly moving and deleting code. See inline comments.
Motivation
https://jira.corp.stripe.com/browse/CAX-3739
https://jira.corp.stripe.com/browse/CAX-3803
Testing
Screenshots
Screen.Recording.2025-03-06.at.12.26.39.PM-compressed.mov