-
Notifications
You must be signed in to change notification settings - Fork 937
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
Preparations for UI Experiments #5627
base: develop
Are you sure you want to change the base?
Preparations for UI Experiments #5627
Conversation
8c62cd4
to
33450e9
Compare
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.
We also need to handle the DARK_EXPERIMENT
value in
Android/common/common-ui/src/main/java/com/duckduckgo/common/ui/DuckDuckGoActivity.kt
Lines 90 to 102 in 3acaa58
fun isDarkThemeEnabled(): Boolean { | |
return when (themingDataStore.theme) { | |
DuckDuckGoTheme.SYSTEM_DEFAULT -> { | |
val uiManager = getSystemService(Context.UI_MODE_SERVICE) as UiModeManager | |
when (uiManager.nightMode) { | |
UiModeManager.MODE_NIGHT_YES -> true | |
else -> false | |
} | |
} | |
DARK -> true | |
else -> false | |
} | |
} |
@@ -89,6 +74,8 @@ fun AppCompatActivity.getThemeId(theme: DuckDuckGoTheme): Int { | |||
return when (theme) { | |||
SYSTEM_DEFAULT -> getSystemDefaultTheme() |
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.
getSystemDefaultTheme
also needs to be updated to return either the regular or experiment themes.
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.
During internal testing we only want to change Light / Dark. Users on system theming won’t be able to see the experimental colors.
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.
This sounds somewhat counter-intuitive for me. The new flag governs whether the experimental light/dark theme is applied, we shouldn't require from internal users any additional implicit steps. The default setting is the "system theme", and in the end it applies one of the light or dark themes, so in my opinion it should respect the experiment flag as well.
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.
"we shouldn't require from internal users any additional implicit steps”. this is still to be decided, for now we will keep it simple and iterate in the project.
app/src/main/java/com/duckduckgo/app/appearance/AppearanceActivity.kt
Outdated
Show resolved
Hide resolved
...n-ui-experiments/src/main/java/com/duckduckgo/common/ui/experiments/BrowserThemingFeature.kt
Show resolved
Hide resolved
common/common-ui/src/main/java/com/duckduckgo/common/ui/store/ThemingSharedPreferences.kt
Show resolved
Hide resolved
33450e9
to
115e825
Compare
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.
One more small change needed:
Android/app/src/main/java/com/duckduckgo/app/appearance/AppearanceActivity.kt
Lines 170 to 180 in 3acaa58
val currentTheme = theme.getOptionIndex() | |
RadioListAlertDialogBuilder(this) | |
.setTitle(R.string.settingsTheme) | |
.setOptions( | |
listOf( | |
R.string.settingsSystemTheme, | |
R.string.settingsLightTheme, | |
R.string.settingsDarkTheme, | |
), | |
currentTheme, | |
) |
currentTheme
should be mapped back to light/dark when experimental is provided. Otherwise, nothing is pre-selected when the experiment is enabled and radio menu opens.
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.
Thanks for addressing the comments, let's merge and continue but please cut follow up tasks for:
if not addressed here.
Task/Issue URL: https://app.asana.com/0/488551667048375/1209289914504437
Description
Steps to test this PR
Enable Experimental UI
Enable Experimental UI