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

Add ColorScheme environment #136

Merged
merged 41 commits into from
Aug 2, 2020
Merged

Add ColorScheme environment #136

merged 41 commits into from
Aug 2, 2020

Conversation

MaxDesiatov
Copy link
Collaborator

@MaxDesiatov MaxDesiatov commented Jun 30, 2020

Was temporarily blocked by #135.

@MaxDesiatov MaxDesiatov added the SwiftUI compatibility Tokamak API differences with SwiftUI label Jun 30, 2020
}

struct ColorSchemeKey: EnvironmentKey {
static let defaultValue: ColorScheme = .light
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible for this to match the value of the (prefers-color-scheme: dark) media query? You could create the query with window.matchMedia('(prefers-color-scheme: dark)') and add an event listener to update this value on change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's what I'm working right now. I think it would make sense to add the listener in an onAppear closure on the new DOMEnvironment view introduced in this PR, but this obviously needs a working onAppear implementation first, which is what I'm working on right now 🙂

MaxDesiatov added a commit that referenced this pull request Jul 2, 2020
This is required to unblock #136 as I think it would make sense for the `DOMEnvironment` view there to add/remove its color scheme listener in `onAppear`/`onDisappear` closures.

I've also restored full SwiftUI compatibility in the signature of `func modifier<Modifier>(_ modifier: Modifier)`. Consequently `ViewDeferredToRenderer` had to be implemented on `ModifiedContent` then instead of `_ViewModifier_Content` to make it work.
Copy link
Member

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

Nice to see it automatically pick up the scheme 🔥

Sources/TokamakDOM/Environment/DOMEnvironment.swift Outdated Show resolved Hide resolved
@MaxDesiatov
Copy link
Collaborator Author

Yes, the listener works, but environment changes aren't propagated properly yet, I'm looking into that now.

@carson-katri
Copy link
Member

Is this unblocked since #170 ?

# Conflicts:
#	Sources/TokamakDOM/DOMRenderer.swift
#	Sources/TokamakDOM/Modifiers/Effects/DOMEnvironment.swift
#	Sources/TokamakDOM/Tokens/Tokens.swift
#	Sources/TokamakDemo/EnvironmentDemo.swift
@MaxDesiatov
Copy link
Collaborator Author

I'm still seeing some issues with environment propagation (at least in this PR), working on that right now...

@MaxDesiatov MaxDesiatov changed the title Add ColorScheme environment Add ColorScheme environment Jul 22, 2020
diskzero and others added 7 commits July 24, 2020 09:05
# Conflicts:
#	Sources/TokamakCore/MountedViews/MountedCompositeView.swift
#	Sources/TokamakCore/MountedViews/MountedHostView.swift
#	Sources/TokamakCore/MountedViews/MountedView.swift
#	Sources/TokamakCore/StackReconciler.swift
#	Sources/TokamakDOM/DOMRenderer.swift
#	Sources/TokamakDemo/TokamakDemo.swift
# Conflicts:
#	Sources/TokamakDOM/DOMRenderer.swift
@MaxDesiatov MaxDesiatov marked this pull request as ready for review August 1, 2020 21:26
@TokamakUI TokamakUI deleted a comment from ie-ahm-robox Aug 1, 2020
@MaxDesiatov
Copy link
Collaborator Author

The last known environment update issue is fixed and it's ready for review. You can see the dark scheme environment text representation updated in EnvironmentDemo. As it has been open for a while now, I suggest adding default dark mode styles in a separate PR, I've created #237 as a reminder for that.

@j-f1
Copy link
Member

j-f1 commented Aug 1, 2020

I gave it a test and it seems like clicking the dark mode toggle in Safari Developer Tools does not work unless you click it before the first render — subsequent changes are not reflected in the view.

@MaxDesiatov
Copy link
Collaborator Author

I was testing with the globally set dark mode in System Preferences, do you see the same behavior with that one?

@j-f1
Copy link
Member

j-f1 commented Aug 1, 2020

Yes, I see the same behavior there

@MaxDesiatov
Copy link
Collaborator Author

Hm, I had this same issue caused by the environment bug, which was fixed in c397eb6. Maybe you're not on the latest commit of this branch?

@carson-katri
Copy link
Member

It's working for me in Chrome. Maybe a bug in Safari?

j-f1
j-f1 previously approved these changes Aug 1, 2020
Copy link
Member

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

Weird, works fine now ¯\_(ツ)_/¯

Sources/TokamakCore/App/_AnyApp.swift Outdated Show resolved Hide resolved
Sources/TokamakDemo/EnvironmentDemo.swift Outdated Show resolved Hide resolved
Sources/TokamakCore/App/App.swift Show resolved Hide resolved
j-f1
j-f1 previously approved these changes Aug 2, 2020
@@ -55,6 +76,8 @@ public typealias CGSize = TokamakCore.CGSize

// MARK: Views

public typealias Button = TokamakCore.Button
Copy link
Member

Choose a reason for hiding this comment

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

I intentionally left out things like Button and TextField as they're pretty much useless without State. Do you think we should include them?

Copy link
Member

Choose a reason for hiding this comment

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

What we can add which will make TokamakStaticHTML more useful is Link

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wonder if we could still render stateful Views to static HTML, any listeners on those would be just ignored. But that would obviously come as a separate PR. LMK if you think that these should be removed in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I think they should be removed, plus they only have HTML bodies in TokamakDOM.

@MaxDesiatov MaxDesiatov merged commit c7b5e75 into main Aug 2, 2020
@MaxDesiatov MaxDesiatov deleted the color-scheme branch August 2, 2020 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SwiftUI compatibility Tokamak API differences with SwiftUI
Development

Successfully merging this pull request may close these issues.

6 participants