Skip to content
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

@W-17411362: LoginActivity Fit and Finish #2668

Conversation

JohnsonEricAtSalesforce
Copy link
Contributor

@JohnsonEricAtSalesforce JohnsonEricAtSalesforce commented Feb 8, 2025

🎸 Ready For Final Review! 🥁

This actually got a lot easier today since #2670 included all the bottom sheet styling for select server and add connection (which looks great. Thanks!)

Note: This has a counterpart in the templates repository: https://github.com/forcedotcom/SalesforceMobileSDK-Templates/pull/444/files

activity.window.decorView.systemUiVisibility = SYSTEM_UI_FLAG_LIGHT_NAVIGATION_BAR or SYSTEM_UI_FLAG_LIGHT_STATUS_BAR
if (SDK_INT > R) {
runCatching {
activity.window?.insetsController?.setSystemBarsAppearance(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brandonpage - Here's a modern way to accomplish what this once did with a dandy to-do so we can clean up once API 30 is our minimum. I did some reading, and though the names have changed the overall approach is almost identical.

@@ -194,6 +195,9 @@ open class LoginActivity: FragmentActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
enableEdgeToEdge()
if (viewModel.dynamicBackgroundTheme.value == Dark) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brandonpage - I added this new dynamicBackgroundTheme state so we can know if the view model considers the background "light" or "dark".

@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2025

Codecov Report

Attention: Patch coverage is 56.62651% with 108 lines in your changes missing coverage. Please review.

Project coverage is 47.40%. Comparing base (e884d94) to head (cf16ae5).
Report is 3 commits behind head on dev.

Files with missing lines Patch % Lines
...m/salesforce/androidsdk/ui/components/LoginView.kt 18.46% 44 Missing and 9 partials ⚠️
.../src/com/salesforce/androidsdk/ui/LoginActivity.kt 22.22% 20 Missing and 1 partial ⚠️
...orce/androidsdk/ui/components/PickerBottomSheet.kt 79.80% 10 Missing and 11 partials ⚠️
...src/com/salesforce/androidsdk/ui/LoginViewModel.kt 69.23% 0 Missing and 4 partials ⚠️
.../salesforce/androidsdk/app/SalesforceSDKManager.kt 50.00% 1 Missing and 2 partials ⚠️
...ce/androidsdk/ui/components/UserAccountListItem.kt 70.00% 2 Missing and 1 partial ⚠️
...src/com/salesforce/androidsdk/ui/theme/SFColors.kt 33.33% 1 Missing and 1 partial ⚠️
...ce/androidsdk/ui/components/LoginServerListItem.kt 95.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #2668      +/-   ##
============================================
+ Coverage     46.49%   47.40%   +0.90%     
- Complexity     2150     2168      +18     
============================================
  Files           206      205       -1     
  Lines         16360    16855     +495     
  Branches       2194     2313     +119     
============================================
+ Hits           7607     7990     +383     
- Misses         7937     7997      +60     
- Partials        816      868      +52     
Components Coverage Δ
Analytics 47.92% <ø> (ø)
SalesforceSDK 33.05% <50.63%> (+2.70%) ⬆️
Hybrid 56.88% <ø> (ø)
SmartStore 78.20% <ø> (ø)
MobileSync 71.89% <ø> (ø)
React 0.00% <ø> (ø)
Files with missing lines Coverage Δ
...force/androidsdk/auth/idp/interfaces/IDPManager.kt 100.00% <100.00%> (ø)
...ce/androidsdk/ui/components/LoginServerListItem.kt 55.43% <95.00%> (ø)
...src/com/salesforce/androidsdk/ui/theme/SFColors.kt 45.07% <33.33%> (+15.56%) ⬆️
.../salesforce/androidsdk/app/SalesforceSDKManager.kt 48.93% <50.00%> (-0.07%) ⬇️
...ce/androidsdk/ui/components/UserAccountListItem.kt 47.25% <70.00%> (ø)
...src/com/salesforce/androidsdk/ui/LoginViewModel.kt 51.58% <69.23%> (-1.81%) ⬇️
.../src/com/salesforce/androidsdk/ui/LoginActivity.kt 14.57% <22.22%> (+0.74%) ⬆️
...orce/androidsdk/ui/components/PickerBottomSheet.kt 55.01% <79.80%> (ø)
...m/salesforce/androidsdk/ui/components/LoginView.kt 44.13% <18.46%> (+3.22%) ⬆️

... and 2 files with indirect coverage changes

@wmathurin
Copy link
Contributor

Can we get rid of sfDarkLoginColors ?

@brandonpage
Copy link
Contributor

Can we get rid of sfDarkLoginColors ?

Unless Eric has evidence to the contrary I'd say yes. I think the overflow menu and login server picker can respect dark theme (as they always have). The header text/icons shouldn't care because they should respond dynamically to the webiew background color.

…igation Bar Theme Relative To Dynamic Top Bar Colors)
…o Enable Applied Theme In Login Composable Functions)
…K Manager Theme For Dynamic Background Theme Values)
@JohnsonEricAtSalesforce JohnsonEricAtSalesforce force-pushed the feature/w-17411362_login_activity-fit-and-finish branch from ff202bb to 6067faa Compare February 12, 2025 04:27
@JohnsonEricAtSalesforce JohnsonEricAtSalesforce marked this pull request as ready for review February 12, 2025 04:51
@brandonpage
Copy link
Contributor

@JohnsonEricAtSalesforce Things I was expecting to see in this PR:

  • Bio Auth / IDP button(s) with Fit and Finish
    • Should we implement a QR Code button so apps don't have to do it in their code?
    • Can we just use additionalBottomBarButtons for all of these?
    • What is the max number that should be shown at once?
  • Fix/more consistent loading animation. Last I checked this did not trigger every time.
  • Accessibility testing. If no work is needed that is fine.
  • Compose Semantic UI Automation -- I can implement this with the customization story if you want since that one should really require it.

@@ -1,15 +1,15 @@
<?xml version="1.0" encoding="utf-8"?>
<resources>

<!-- Errors -->
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The leading indentation for this source was pretty mixed, so since we're adding a significant number of lines I thought a gentle reformat to Android Studio's defaults would be helpful.

@@ -117,7 +120,7 @@ fun LoginServerListItem(
},
label = "offset"
)
var rowSizePixels by remember { mutableStateOf(IntSize(0,0)) }
var rowSizePixels by remember { mutableStateOf(IntSize(0, 0)) }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also gave the login composables a reformat to Android Studio's defaults.

@JohnsonEricAtSalesforce
Copy link
Contributor Author

@JohnsonEricAtSalesforce Things I was expecting to see in this PR:

  • Bio Auth / IDP button(s) with Fit and Finish

    • Should we implement a QR Code button so apps don't have to do it in their code?
    • Can we just use additionalBottomBarButtons for all of these?
    • What is the max number that should be shown at once?
  • Fix/more consistent loading animation. Last I checked this did not trigger every time.

  • Accessibility testing. If no work is needed that is fine.

  • Compose Semantic UI Automation -- I can implement this with the customization story if you want since that one should really require it.

I'm getting these entered in GUS as well. We had some but not all of these under our general finish work item.

@JohnsonEricAtSalesforce
Copy link
Contributor Author

  • Should we implement a QR Code button so apps don't have to do it in their code?

So much of QR Code login is customizable according to the implementation we released in 12.x, including the placement and function of this button, that I feel it belongs to the app. The real kicker in my view there is that MSDK doesn't control what that button actually does. Our template app's example uses one version of image capture logic but each implementation can completely customize that.

It's also nice that with the additionalBottomBarButtons view model customization it's a really tidy one-liner to add this button there, if the app wishes to use our template's approach.

Perhaps our guiding logic should be to add MSDK-provided buttons for features where MSDK controls the logic they expose and let apps use additionalBottomBarButtons for anything the app provides the logic for?

…gin components except the show menu button placed over dynamic color)
…cation Button, Set Up Biometric Authentication Button And Overall Cleanup Of Login Bottom Bar Buttons)
@JohnsonEricAtSalesforce JohnsonEricAtSalesforce merged commit 3a3951e into forcedotcom:dev Feb 14, 2025
6 checks passed
@JohnsonEricAtSalesforce JohnsonEricAtSalesforce deleted the feature/w-17411362_login_activity-fit-and-finish branch February 14, 2025 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants