Generic LoginServer / UserAccount Picker Bottom Sheet #2670
Conversation
…ivity reference from LoginView, LoginWebview and LoginWebviewClient.
…del. Add a view model factory var on SDKMangager so LoginActivity can be customized without overriding.
…ow WebChromeClient to be passed into LoginView.
…ve unnecessary passing of LoginViewModelFactory.
…_credentials.json for API 35.
…r onCancel picker parameter. Prevent duplicate servers.
…oid into 13.0-modern-login # Conflicts: # libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/LoginViewModelTest.kt
…heet. Remove all magic numbers.
…t to show duplicates. Cleanup UserAccountListItem.
Generated by 🚫 Danger |
Generated by 🚫 Danger |
| // Prevent duplicate servers. | ||
| for (LoginServer existingServer : getLoginServers()) { | ||
| if (name.equals(existingServer.name) && url.equals(existingServer.url)) { | ||
| setSelectedLoginServer(existingServer); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
It did not make sense to check for this in the UI code. If we did, we would then have to surface some kind of error message. Simply not adding the duplicate and selecting the original in the data source of truth seemed like an elegant solution.
And I purposefully did not use our equals method because I wanted to ignore isCustom.
| * | ||
| * @param server the server to remove | ||
| */ | ||
| public void removeServer(LoginServer server) { |
There was a problem hiding this comment.
This was a bit gnarly, but necessary for the new UX.
| val onNewLoginServerSelected = { newSelectedServer: Any?, closePicker: Boolean -> | ||
| if (newSelectedServer != null && newSelectedServer is LoginServer) { | ||
| viewModel.showServerPicker.value = !closePicker | ||
| viewModel.loading.value = true | ||
| viewModel.dynamicBackgroundColor.value = backgroundColor | ||
| SalesforceSDKManager.getInstance().loginServerManager.selectedLoginServer = newSelectedServer | ||
| } | ||
| } | ||
| val onLoginServerCancel = { viewModel.showServerPicker.value = false } | ||
| val onUserAccountSelected = { userAccount: Any?, _: Boolean -> | ||
| if (userAccount != null && userAccount is UserAccount) { | ||
| activity?.finish() | ||
| userAccountManager.switchToUser(userAccount) | ||
| } | ||
| } | ||
| val onUserSwitchCancel = { | ||
| activity?.finish() | ||
| if (userAccountManager.currentUser == null) { | ||
| userAccountManager.switchToUser(userAccountManager.authenticatedUsers.first()) | ||
| } | ||
| } | ||
| val sheetState = rememberModalBottomSheetState( | ||
| skipPartiallyExpanded = true, | ||
| confirmValueChange = { sheetValue -> | ||
| if (sheetValue == SheetValue.Hidden) { | ||
| when (pickerStyle) { | ||
| PickerStyle.LoginServerPicker -> onLoginServerCancel() | ||
| PickerStyle.UserAccountPicker -> onUserSwitchCancel() | ||
| } | ||
| } | ||
|
|
||
| true | ||
| } | ||
| ) | ||
| val addNewLoginServer = { name: String, url: String -> | ||
| loginServerManager.addCustomLoginServer(name, url) | ||
| viewModel.showServerPicker.value = false | ||
| viewModel.loading.value = true | ||
| viewModel.dynamicBackgroundColor.value = backgroundColor | ||
| } |
There was a problem hiding this comment.
These inline functions are the only untested part of the code. My UI tests check that they are called but their actual contents can't be tested. Maybe the Login Server ones should be added to LoginViewModel?
The UserAccount ones wouldn't belong there and require reference to the activity (which should be avoided in view models IMO).
| color = colorScheme.surfaceVariant, | ||
| ) | ||
|
|
||
| // Add New Connection/Account Button |
There was a problem hiding this comment.
Adding this to the end of the list feels a bit obtuse, but having the add button as the last list item really helps to make the UI more usable on a phone in landscape when the list is very cramped.
| // Ensure no duplicates in the list because they are not allowed by lazy column. | ||
| fun List<Any>.pickerDistinctBy() : List<Any> { | ||
| return distinctBy { listItem -> | ||
| when(listItem) { | ||
| is LoginServer -> with(listItem) { "$name$url" } | ||
| else -> listItem.toString() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is probably necessary/overkill since the lazy column uses the list items's toString as a unique key but I would rather just not show duplicate servers at all if they are already stored on the users device pre-13.0.
libs/SalesforceSDK/src/com/salesforce/androidsdk/config/LoginServerManager.java
Outdated
Show resolved
Hide resolved
… adjust tests as a result. Update picker subtext color.
| ) | ||
| } | ||
|
|
||
| @Composable | ||
| fun sfDarkColors(): ColorScheme { | ||
| val context = LocalContext.current | ||
| return darkColorScheme( | ||
| primary = Color(SFColors.primaryColor(context)), | ||
| primaryContainer = Color(SFColors.primaryColorDark(context)), | ||
| primary = Color(SFColors.primaryColorDark(context)), |
PickerBottomSheetis an extremely DRY, reusable container that displays a row list item component that corresponds with aPickerStyleenum value. If we want to add a new type of Picker in the future, it should be as easy as adding a new enum value and filling in the places the compiler tells you are not exhaustive.The public component does a lot of setup work with SalesforceSDKManager, LoginViewModel, etc to call a secondary internal component of the same name. This is done to facilitate previews and to make the component testable. For example: the internal picker takes in an
onItemSelected: (Any?, Boolean) -> Unitparameter which can be supplied with nothing for previews and a function that tests if is actually being called in tests.Combining these two approaches makes the picker quite abstract but I believe though careful naming the code is still very readable (let me know if you disagree!).
LoginServerListItem Preview Example:
UserAccountListItem Preview Example:
AddConnection Preview:
PickerBottomSheet Preview:
