[MOB-2561] Ecosia theme review and fixes #681
Conversation
| if LegacyThemeManager.instance.systemThemeIsOn { | ||
| let userInterfaceStyle = UIScreen.main.traitCollection.userInterfaceStyle | ||
| LegacyThemeManager.instance.current = userInterfaceStyle == .dark ? LegacyDarkTheme() : LegacyNormalTheme() | ||
| } | ||
|
|
||
| changeCurrentTheme(loadInitialThemeType()) |
There was a problem hiding this comment.
It feels like what you do here is exactly what should happen inside changeCurrentTheme(loadInitialThemeType()). I guess this has to do with the cold start, but any reason why not keeping just one of the two?
There was a problem hiding this comment.
@lucaschifino thanks for asking!
While it may look the same, a small check makes (on our codebase with our references of the LegacyThemeManager throughout it) the whole logic behaves unexpectedly (from the User perspective), setting the wrong theme.
Within loadInitialThemeType(), a check of a persisted value is made 👇
let savedThemeDescription = userDefaults.string(forKey: ThemeKeys.themeName).
This check will extract the last saved theme before killing the app, therefore a different one in case we switched at app killed state where no observers were alive.
There was a problem hiding this comment.
Got it! It's ok to leave it like that by me, but maybe making that a bit clearer for people reading the code would be good, either a comment like // Additional check in case the theme changed while the app was closed on top of your code or renaming the existing method to changeCurrentTheme(loadInitialStoredThemeType()).
MOB-2561
Context
Throughout the testing of the latest upgrade version, some Theming issues arose.
This PR aims to fix all of them as well as provide a bit of touch to some details already present in production.
Approach
Also acknowledged the root cause of the theming issue when the app performs a cold start. The issue behind the
LegacyThemeManagerthat needed another entry point in this current codebase and the fact that our landing is theWelcomescreen / onboarding.GIF showcasing the fix for all of them
Other
The UI theming applied to the Done button is a personal proposal. The Done button is Green throughout the app in dark mode so I thought would be good to align it.
Before merging
Checklist
// Ecosia:helper comments where needed