Enhancement: Improved login UX#108
Open
sockenklaus wants to merge 11 commits into
Open
Conversation
This commit introduces a basic login screen that will be shown when the user is not logged in. The `MainViewModel` now observes the login state from `UserRepository` and sets the appropriate start destination (`BookmarkListRoute` or `LoginRoute`). The `MainActivity` is updated to use this dynamic start destination for the `NavHost`. A new `LoginScreen.kt` and `LoginScreenViewModel.kt` have been added to handle the UI and logic for the login screen. A new `LoginRoute` has been added to `Routes.kt`.
…me first business logic to `LoginScreenViewModel.kt`
This commit introduces several improvements to the login screen: - Implement password visibility toggle - Change HTTP/HTTPS prefix based on "allow unencrypted connection" - Add ExtendedFloatingActionButton for login - Update ViewModel to handle input changes (Work in Progress!)
The login button on the login screen now displays a login icon.
This commit introduces input validation for the URL, username, and password fields on the login screen.
- **LoginScreenViewModel:**
- Added `validateUrl`, `validateUsername`, and `validatePassword` methods to check for empty or invalid inputs.
- `LoginUiState` now includes `urlError`, `usernameError`, and `passwordError` fields to hold validation error message resource IDs.
- The `loginEnabled` property in `LoginUiState` is now computed based on whether all error fields are null.
- Added `onToggleUseApiToken` to switch between password and API token input, updating relevant error messages and password visibility.
- Added `onClickLogin` which currently does nothing if login is not enabled.
- **LoginScreen.kt:**
- TextFields now display error messages from `uiState.urlError`, `uiState.usernameError`, and `uiState.passwordError` using `stringResource`.
- The password/API token label dynamically changes based on `uiState.useApiToken`.
- A Switch has been added to toggle between using a password and an API token.
- The username field is now hidden when `useApiToken` is true.
- The `onClick` handler for the login `ExtendedFloatingActionButton` now calls `viewModel.onClickLogin()`.
- **strings.xml:**
- Added `account_settings_url_empty_error` and `account_settings_apitoken_error` for new validation messages.
- Renamed "Allow unencrypted connections" to "Use unencrypted connections" for clarity.
- Differentiated network errors from other errors by introducing `UserRepository.LoginResult.NetworkError`.
- Improved error parsing for `StatusMessageDto` to handle potential `SerializationException`.
- **AuthenticateUseCase:**
- Distinguishes between generic errors, network errors, and authentication failures (HTTP 403).
- **LoginScreenViewModel:**
- Updated to handle the new `AuthenticationResult` states from `AuthenticateUseCase`.
- `onToggleAllowUnencryptedConnection` renamed to `onToggleUseUnencryptedConnection`.
- Login button now validates all fields before attempting login.
- If URL doesn't end with `/api`, it's automatically appended.
- `UiState.loginEnabled` logic updated to correctly reflect when username is not required (e.g., API token usage).
- Added `onAuthenticationResultConsumed` to clear authentication results after they are handled (e.g., shown in a Snackbar).
- **LoginScreen:**
- Displays specific error messages for authentication failures (wrong credentials) and network errors (unreachable instance) directly in the relevant input fields.
- Generic errors are now shown in a Snackbar.
- Renamed "Allow unencrypted connection" to "Use unencrypted connection".
- `ExtendedFloatingActionButton` for login is now centered without using a Box() element.
- **StatusMessageDto:**
- Made fields `statusField` and `statusCodeField` nullable and added a transient `status` property.
- **strings.xml:**
- Added new string resources for authentication failure and network error messages.
- A `isLoading` state is added to `LoginScreenUiState` to indicate when a login attempt is in progress.
- The `onClickLogin` function in `LoginScreenViewModel` now sets `isLoading` to true before attempting login and false afterwards.
- The login button in `LoginScreen` now displays a `CircularProgressIndicator` when `isLoading` is true and is disabled during this time.
- **AccountSettingsViewModel & AccountSettingsScreen:**
- A `logout()` function is added to `AccountSettingsViewModel` (currently a placeholder).
- The `AccountSettingsScreen` is refactored to display account information as non-editable text fields with icons, rather than `OutlinedTextField`s.
- A "Logout" `ExtendedFloatingActionButton` is added to the `AccountSettingsScreen` instead of the current "Login" button.
- **AccountSettingsUiState:**
- A `useApiToken` boolean field is added.
- Corrected the logic for enabling the login button (`loginEnabled`) to ensure all necessary fields are valid and filled.
- **LoginScreenViewModelTest:**
- Started on adding unit tests for `LoginScreenViewModel`.
- **AccountSettingsViewModel:**
- Implemented the `logout()` function, which now utilizes the new `LogoutUseCase`.
- **LogoutUseCase:**
- Introduced a new `LogoutUseCase` responsible for clearing credentials, deleting all bookmarks, and resetting the last bookmark timestamp.
- **MainActivity & MainViewModel:**
- Changed the `initialValue` of `startDestination` to `null`
- Ensured `ReadeckNavHost` is only composed when `startDestination` is not null to prevent the wrong Composable from showing when it's not clear whether the user is logged in or not
- Removed `navHostController` parameter from `LoginScreen` composable as it's not directly used.
- **UserRepositoryImplTest:**
- Updated test cases to use the named arguments for `StatusMessageDto` to reflect the added fields in `StatusMessageDto`
- Changed `LoginResult.Error` to `LoginResult.NetworkError` in a test case to reflect a more specific error type.
- **BookmarkRepositoryImplTest:**
- Updated test cases to use named arguments for `StatusMessageDto`.
…gsScreen login logic
This commit refactors string resources related to login functionality by:
- Removing login-specific strings from `account_settings_*.xml`.
- Adding new, prefixed strings for the `LoginScreen` in `strings.xml` (e.g., `login_url_label`, `login_password_label`).
- Updating translations in `values-de-rDE`, `values-es-rES`, and `values-zh-rCN` to remove the old account settings login strings.
Additionally, login-related logic and UI elements have been removed from `AccountSettingsScreen.kt` and its corresponding ViewModel and tests (`AccountSettingsViewModel.kt`, `AccountSettingsViewModelTest.kt`). This includes:
- Removal of UI state related to login (e.g., `loginEnabled`, `urlError`, `authenticationResult`).
- Removal of methods for handling login input changes and the login action itself.
- Removal of related tests.
Updated Jetpack Compose to 1.8.2 to use Modifier.semantics for AutoFill functionality.
Other changes:
- `LoginScreen.kt`:
- Updated to use the new `login_*` string resources.
- Set `contentType` semantics for username and password fields.
- Updated login button icon content description to `null`.
- `LoginScreenViewModel.kt`:
- Updated to use new `login_*` string resources for error messages.
- `onPasswordChanged` renamed to `onPasswordOrApiTokenChanged`.
- `onToggleShowPassword` renamed to `onToggleShowPasswordOrApiToken`.
- Logic for handling API token authentication is still a TODO.
- URL validation in `onClickLogin` now removes http(s) schema if present.
- `LoginScreenViewModelTest.kt`: Added tests for new input handling, state updates, and login logic.
- `BookmarkListViewModel.kt` and `BookmarkListViewModelTest.kt`: Updated to use `list_view_invalid_url_error` string resource for URL validation errors.
Contributor
Author
|
Finally done! =) A few things to consider:
@jensomato, could you please review my changes and let me know your thoughts? |
- sharing links to app did crash the app because it was trying to access NavHost before it was initialized - only navigate to BookmarkList on share if logged in This commit updates `MainActivity.kt` to check the login status before navigating to the `BookmarkListRoute` when a URL is shared with the app. It also only starts to observe `intentState.value` after startDestination has been initialized.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I am implementing my comment from #84 here: I have created a login screen that serves as the default destination when no user is logged into the app.
Done
LoginScreenis set as the start destination if the user is not logged inLoginScreenincludes text fields for URL, username, password/API token as well as switches for "Use unencrypted connection" and "Login with API token".Disable the login button if there are any errors in the formThis is not easily achievable with FAB and doesn't really add to the user experienceAccountSettingsScreento indicate that credentials cannot be edited through this screenAccountSettingsScreen@jensomato If you have the time, could you please take a moment to review whether I'm on the right track?