[DynamicColors] Add support for dynamic colors #4850
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4850 +/- ##
==========================================
- Coverage 83.90% 83.82% -0.08%
==========================================
Files 596 596
Lines 48460 48543 +83
Branches 6680 6691 +11
==========================================
+ Hits 40659 40693 +34
- Misses 5533 5578 +45
- Partials 2268 2272 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great job, no security vulnerabilities found in this Pull Request |
| BitwardenTheme(theme = state.theme) { | ||
| BitwardenTheme( | ||
| theme = state.theme, | ||
| dynamicColor = state.isDynamicColorsEnabled, |
There was a problem hiding this comment.
How did the design team map the colors properly?
They are not one-to-one and we can never guarantee that colors meet accessibility requirements or that everything will even work correctly.
There was a problem hiding this comment.
I added a warning when dynamic colors is enabled so users are aware of the risks.
The way BitwardenColorSchemes is designed gives us enough flexibility that it doesn't need to be 1:1. The only component that we're seeing issue with is the switch, which I can fix with a simple change to the attribute we're mapping to. I think, with a little bit of finesse, we can work around the other concerns of multi-tonal icons and illustrations. Kudos to you for setting it up this way.
To keep concerns separate I'm going to open different PRs with my ideas to tackle those other hurdles.
SaintPatrck
force-pushed
the
innovation-sprint/dynamic-colors/set-enabled
branch
from
005adbf to
763422a
Compare
|
Warning @SaintPatrck Uploading code coverage report failed. Please check the "Upload to codecov.io" step of Process Test Reports job for more details. |
SaintPatrck
force-pushed
the
innovation-sprint/dynamic-colors/set-enabled
branch
5 times, most recently
from
f7ab014 to
e0ec47e
Compare
SaintPatrck
changed the title
| ) | ||
| mutableIsDynamicColorsEnabledFlow.tryEmit(value) | ||
| } | ||
| override val isDynamicColorsEnabledFlow: Flow<Boolean?> |
There was a problem hiding this comment.
Can we get some spaces between these vars
SaintPatrck
force-pushed
the
innovation-sprint/dynamic-colors/set-enabled
branch
from
e0ec47e to
fbebe6f
Compare
SaintPatrck
changed the base branch from
innovation-sprint/dynamic-colors/base
to
main
SaintPatrck
force-pushed
the
innovation-sprint/dynamic-colors/set-enabled
branch
2 times, most recently
from
5f7f328 to
1584325
Compare
| @@ -120,6 +124,17 @@ class FakeSettingsDiskSource : SettingsDiskSource { | |||
| get() = mutableAppThemeFlow.onSubscription { | |||
| emit(appTheme) | |||
| } | |||
| override var isDynamicColorsEnabled: Boolean? | |||
There was a problem hiding this comment.
Can we add a new line above this
This commit introduces support for dynamic colors in the application. - A new setting `isDynamicColorsEnabled` is added to `SettingsDiskSource` and `SettingsRepository` to persist and retrieve the dynamic colors preference. - A new flow `isDynamicColorsEnabledFlow` is added to track the changes of `isDynamicColorsEnabled`. - A new switch "Dynamic colors" is added in the Appearance Screen. - The `MainState` is updated with the `isDynamicColorsEnabled` attribute. - `BitwardenTheme` is updated to set the dynamic color. - The `AppearanceViewModel` is updated to set the `isDynamicColorsEnabled`.
- Changed the toggle button switch color to `onPrimary` in the `dynamicColorScheme`.
This commit introduces a warning dialog that is displayed when the user attempts to enable dynamic colors in the appearance settings.
SaintPatrck
force-pushed
the
innovation-sprint/dynamic-colors/set-enabled
branch
from
1584325 to
a9fc13d
Compare
🎟️ Tracking
📔 Objective
Introduce support for dynamic colors in the application.
isDynamicColorsEnabledis added toSettingsDiskSourceandSettingsRepositoryto persist and retrieve the dynamic colors preference.isDynamicColorsEnabledFlowis added to track the changes ofisDynamicColorsEnabled.MainStateis updated with theisDynamicColorsEnabledattribute.BitwardenThemeis updated to set the dynamic color.AppearanceViewModelis updated to set theisDynamicColorsEnabled.📸 Screenshots
Coming soon!
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes