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

Theming for new compose classes #2665

Merged
merged 7 commits into from
Feb 7, 2025
Merged

Conversation

wmathurin
Copy link
Contributor

No description provided.

import androidx.core.content.ContextCompat
import com.salesforce.androidsdk.R

object SFColors {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That way, the colors xml is still being used (and can be overriden in the client app). Or we don't want to do it this way ?? @brandonpage @JohnsonEricAtSalesforce

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind it, but I am also not sure if it servers a purpose. I don't think even the internal apps are using our existing colors for anything other than overriding our UI.

If we keep the XML colors do I have to add new values there and here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I don't keep it, how would they override the colors?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was originally thinking via the view model but I don't know what is best. My first thought is that the theme could be a data class (that could be overridden) instead of an object? 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

Or since we seem to be using a lot of Material3 components, should we be using a MaterialTheme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe - should we provide an easy way for an application to use a different theme ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we allow the app to provide MaterialTheme's for light and dark (through SalesforceSDKManager) and we would default to the MaterialTheme.lightColorTheme() and MaterialTheme.darkColorTheme() ??

Copy link
Contributor

Choose a reason for hiding this comment

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

@brandonpage & @wmathurin - Did you come up with an answer on the approach for this? I believe we're keeping the colors in the resource XML?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are keeping sf__colors.xml until at least 14.
So that's one way to override default colors.
We also added setDarkColorScheme() and setLightColorScheme() to SalesforceSDKManager - as another way to override colors.
Finally for login, the LoginViewModel exposes a few key colors as members.

As we work on login fit/finish, we might simplify things a bit??

Copy link

github-actions bot commented Feb 3, 2025

1 Warning
⚠️ libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/idp/IDPAuthCodeActivity.kt#L117 - Using setJavaScriptEnabled can introduce XSS vulnerabilities into your application, review carefully

Generated by 🚫 Danger

import androidx.compose.ui.platform.LocalContext

enum class SalesforceThemeType {
Light, Dark, DarkLogin
Copy link
Contributor

Choose a reason for hiding this comment

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

Does DarkLogin exist because we can't change the webview background color from white? What I had been doing was hiding the WebView (via opacity) when loading so we could set the background color to whatever we want -- not sure that is relevant though.

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 think that was the reason for DarkLogin.
Let me try with Dark and see how it looks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the experience using Dark (instead of DarkLogin)

LoginWithDark

@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2025

Codecov Report

Attention: Patch coverage is 22.50000% with 62 lines in your changes missing coverage. Please review.

Project coverage is 50.80%. Comparing base (8ab96fc) to head (cb07558).
Report is 6 commits behind head on dev.

Files with missing lines Patch % Lines
...src/com/salesforce/androidsdk/ui/theme/SFColors.kt 29.50% 42 Missing and 1 partial ⚠️
.../salesforce/androidsdk/app/SalesforceSDKManager.kt 0.00% 8 Missing and 2 partials ⚠️
...esforce/androidsdk/auth/idp/IDPAuthCodeActivity.kt 0.00% 4 Missing ⚠️
...rc/com/salesforce/androidsdk/ui/DevInfoActivity.kt 0.00% 3 Missing ⚠️
.../src/com/salesforce/androidsdk/ui/LoginActivity.kt 0.00% 0 Missing and 1 partial ⚠️
...om/salesforce/androidsdk/ui/ManageSpaceActivity.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #2665      +/-   ##
============================================
- Coverage     56.41%   50.80%   -5.61%     
+ Complexity     2468     2300     -168     
============================================
  Files           206      206              
  Lines         16282    16375      +93     
  Branches       2187     2195       +8     
============================================
- Hits           9185     8320     -865     
- Misses         6132     7164    +1032     
+ Partials        965      891      -74     
Components Coverage Δ
Analytics 48.14% <ø> (+0.21%) ⬆️
SalesforceSDK 30.31% <22.72%> (-11.16%) ⬇️
Hybrid 57.28% <ø> (ø)
SmartStore 78.20% <ø> (ø)
MobileSync 82.02% <ø> (ø)
React 52.36% <ø> (ø)
Files with missing lines Coverage Δ
.../src/com/salesforce/androidsdk/ui/LoginActivity.kt 13.83% <0.00%> (ø)
...om/salesforce/androidsdk/ui/ManageSpaceActivity.kt 0.00% <0.00%> (ø)
...rc/com/salesforce/androidsdk/ui/DevInfoActivity.kt 0.00% <0.00%> (ø)
...esforce/androidsdk/auth/idp/IDPAuthCodeActivity.kt 0.00% <0.00%> (ø)
.../salesforce/androidsdk/app/SalesforceSDKManager.kt 49.00% <0.00%> (-2.08%) ⬇️
...src/com/salesforce/androidsdk/ui/theme/SFColors.kt 29.50% <29.50%> (ø)

... and 28 files with indirect coverage changes

Providing a way for the application to provide its own color schemes
@wmathurin
Copy link
Contributor Author

@brandonpage @JohnsonEricAtSalesforce what do you think of this approach (5b18ebe) for theming?

…d into themes

# Conflicts:
#	libs/SalesforceSDK/src/com/salesforce/androidsdk/app/SalesforceSDKManager.kt
@wmathurin
Copy link
Contributor Author

Should we merge this one?
There is probably be some rework when we do the login fit and finish work e.g. where the colors should come from (properties on the LoginViewModel and/or sf__colors.xml and/or dynamic).
@JohnsonEricAtSalesforce @brandonpage

@wmathurin wmathurin marked this pull request as ready for review February 6, 2025 22:13
Copy link
Contributor

@JohnsonEricAtSalesforce JohnsonEricAtSalesforce left a comment

Choose a reason for hiding this comment

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

🤘🏻

@wmathurin wmathurin merged commit 33f7c5c into forcedotcom:dev Feb 7, 2025
6 checks passed
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