Skip to content
This repository has been archived by the owner on Jan 2, 2024. It is now read-only.

Add theming to Demo app #7

Merged
merged 2 commits into from
May 9, 2023
Merged

Add theming to Demo app #7

merged 2 commits into from
May 9, 2023

Conversation

tonisevener
Copy link
Collaborator

Phabricator:
N/A

Notes

This adds some basic theme selection / persistence to the demo app, and adds paperBackground as a color. This will help confirm our UI is theming correctly as we build out native editor.

Test Steps
Confirm theme selection persists between launches and works as expected. Confirm the Canvas background reflects the paperBackround color, and the default theme selection switches between light and black based on the system dark mode.

@tonisevener tonisevener requested review from a team and staykids and removed request for a team May 4, 2023 21:59
@staykids staykids self-assigned this May 9, 2023
Copy link
Contributor

@staykids staykids left a comment

Choose a reason for hiding this comment

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

Nice!

Don't sweat the tabs/spaces stuff, I'll try to get the SwiftLint stuff in sooner rather than later to try and resolve these issues for us. I'm not immediately clear if there's a way to have a Swift Package default to one or the other the same way an xcodeproj can though...

return string(forKey: WKThemeNameKey)
}
set {
set(newValue, forKey: WKThemeNameKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not suggesting we change anything here, but just want to point out something odd. In the Simulator, if you change the theme, then kill the app within a few seconds, the theme doesn't always persist and sometimes reverts to previous selection on next launch. Per the docs When you set a default value, it’s changed synchronously within your process, and asynchronously to persistent storage and other processes., so I think what's happening is understandable (process killed before it asynchronously saves to store, so UserDefaults uses its cache on next launch) and behaviorally acceptable. I think incorrectly using setValue(..., ...) resolves it, but again, not suggesting we change anything just wanted to point out something weird 🥴.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Strange! I haven't been able to repro myself, but that does sound like exactly what's happening. Do you see a similar issue in the main app? Looks like this method was a straight copy from NSUserDefaults+WMFExtensions (complete with an unnecessary @objc, my mistake!).

Copy link
Contributor

Choose a reason for hiding this comment

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

@tonisevener I tried to replicate a few times in the main app, but no luck. Here's what it looks like in this demo app for me in the Simulator:

  1. Fresh run, theme is Default
  2. Update theme to Sepia
  3. Tap run again in Xcode
  4. Theme is still Default
theme_defaults.mov

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah thanks - I was testing by terminating via the app switcher. What's interesting is by following your steps (terminating by stopping in Xcode, I assume), it won't save unless I wait 5+ seconds before stopping. But if I terminate through the app switcher (something which only takes me about 2 seconds) it seems to save immediately. I wonder if under the hood iOS saves any pending UserDefaults set requests when you go through app switcher. I've been doing this through an old device which may also cause differences.

Anyways, I guess it's fine, though strange that our main app doesn't have this problem. That makes me feel like we're missing something obvious.

import UIKit

extension WKAppEnvironment {
static func updateWithTraitCollection(_ traitCollection: UITraitCollection) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is useful and should maybe even eventually be rolled into WKAppEnvironment itself at some point in some form. But let's feel it out for a bit first. For some reason I feel like we should avoid extending the Components classes from the client app unless it feels hyper client specific, and theme and colors are kind of a gray in my mind because we define them in the Components framework itself.

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah good point! I think this came more from not wanting our main app to have access to this logic, because it already has it's own form of this logic in this UserDefaults extension method, and I didn't want to mess with that. But for sure it's very annoying that this logic is duplicated in a different way there - I think a good path may be to move this method into Components and clean up the client app to depend on this logic. Then (if necessary for code change blowup) we could have a syncing method in our main app that ensures the WKTheme setting also sets the correct WMFTheme, instead of the other way around.

@staykids staykids merged commit 7ed24c3 into main May 9, 2023
@staykids staykids deleted the theme-demo branch May 9, 2023 23:16
@staykids staykids removed their assignment May 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants