-
Notifications
You must be signed in to change notification settings - Fork 645
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
Networking Sign Up Pane (Native): Add back auto-focus #8557
base: master
Are you sure you want to change the base?
Networking Sign Up Pane (Native): Add back auto-focus #8557
Conversation
Diffuse output:
APK
DEX
|
modifier = modifier | ||
.imePadding(), |
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.
hoisted imePadding a bit up so that users can customize it, and adds it for the Elements bottom sheet to keep current behavior.
…muvi/BANKCON-10945/networking-sign-up-pane-native-add-back-auto-focus
…muvi/BANKCON-10945/networking-sign-up-pane-native-add-back-auto-focus
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.
Looks good!
|
||
// When pane loads, focus on email field if it's empty |
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.
Nit: I don’t think we need these comments, as the code already explains it :)
internal inline fun Modifier.conditional( | ||
condition: Boolean, | ||
ifTrue: Modifier.() -> Modifier, | ||
ifFalse: Modifier.() -> Modifier = { this }, |
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.
Nit: We don’t seem to use ifFalse
. Should we rename this to fun Modifier.ifTrue(condition, action)
?
@@ -47,6 +49,7 @@ import com.stripe.android.financialconnections.ui.theme.FinancialConnectionsThem | |||
* @param inModal whether the layout is being used in a modal or not. If true, the [body] won't expand to fill the | |||
* available content. | |||
* @param showFooterShadowWhenScrollable whether to show a shadow at the top of the footer when the body is scrollable. | |||
* @param applyImePadding whether to apply IME padding. When false, footer will be covered by the keyboard. |
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.
Nit: Should we rename this to keepFooterAboveKeyboard
or something more concrete than applyImePadding
?
emailFocusRequester.requestFocus() | ||
} | ||
} | ||
// Everytime full form is shown, scroll to bottom and focus on phone number field (if not empty) |
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.
Same here.
Summary
no_prefill.mp4
email_prefill.mp4
both_filled.mp4
Motivation
📔 Networking Sign Up Pane (Native): Add back auto-focus
🌐 BANKCON-10945
Testing
Screenshots
Changelog