-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: Improve app password required dialog #22196
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
Conversation
Remove from abstract class to fix Jetpack Compose preview error: "render error."
Preview the actual UI code used by the app, rather than a copy of the dialog composition.
Communicate the dialog may be dismissed.
While Material's guidlines state to avoid unnecessary punctuation, all examples that have one more than one sentence use punctuation for all sentences.
Generated by 🚫 Danger |
|
|
| App Name | Jetpack |
|
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22196-fe69565 | |
| Commit | fe69565 | |
| Direct Download | jetpack-prototype-build-pr22196-fe69565.apk |
|
| App Name | WordPress |
|
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22196-fe69565 | |
| Commit | fe69565 | |
| Direct Download | wordpress-prototype-build-pr22196-fe69565.apk |
| val isLoading = viewModel.isLoading.collectAsState() | ||
| ApplicationPasswordDialog( | ||
| title = stringResource(getTitleResource()), | ||
| description = getDescriptionString(), | ||
| buttonText = getButtonTextResource(), | ||
| isLoading = isLoading.value, |
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 ViewModel to simplify Jetpack Compose component arguments and preview.
| private fun ApplicationPasswordReauthenticateDialogPreviewContent() { | ||
| val isLoading = remember { mutableStateOf(false) } | ||
|
|
||
| AlertDialog( |
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.
Replaced explicit "preview" component with preview of the actual UI code used by the application.
| <string name="application_password_not_supported_error" a8c-src-lib="module:login">The provided site does not support Application Password authentication.</string> | ||
| <string name="application_password_invalid">Invalid Application Password</string> | ||
| <string name="application_password_invalid_description">Your application password no longer exists. Please sign in again to create a new application password </string> | ||
| <string name="application_password_invalid_description">Your application password no longer exists. Please sign in again to create a new application password.</string> |
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.
While Material guidelines suggest avoiding unnecessary punctuation, all examples with multiple sentences punctuate all sentences.
adalpari
left a comment
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.
Nice code improvement. Thanks!
LGTM!





Tip
Review each commit individually to simplify comprehending the changes.
Description
ApplicationPasswordReauthenticateDialogPreviewto fix JetpackCompose preview
ApplicationPasswordDialogfor Jetpack Compose to preview actual UIcomposition rather than duplicate
ApplicationPasswordDialogdismiss button to communicate ability todismiss the dialog
Testing instructions
Jetpack Compose preview images below showcase the updated visuals. Functional testing
can be performed by following the testing plan outlined in either of the
following: