-
Notifications
You must be signed in to change notification settings - Fork 76
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 a SystemThemeObserver implementation for wasmJs #998
Conversation
@igordmn any chance this gets into 1.6? |
compose/ui/ui/src/wasmJsMain/kotlin/androidx/compose/ui/window/SystemThemeObserver.wasm.kt
Outdated
Show resolved
Hide resolved
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.
LGTM! thanks!
yes, it will get into 1.6 |
this change doesn't affect iOS, so Failing iOS unit tests is not a blocker here. |
// supported by all browsers since 2015 | ||
// https://developer.mozilla.org/en-US/docs/Web/API/Window/matchMedia | ||
@JsFun("() => window.matchMedia != undefined") |
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.
Question out of curiosity: why was this JsFun
hack used instead of accessing the window
global directly?
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.
I believe I did that because the window
global's matchMedia
isn't nullable, and this check is to see if it exists, which I think wouldn't go well with using the global if it actually didn't exist (although I didn't actually test to see if it would crash).
Proposed Changes
SystemThemeObserver
for wasmJsTesting
Test: Built locally and used with a wasmJs compose app, and observed that dark mode was set correctly
Google CLA
Done