-
-
Notifications
You must be signed in to change notification settings - Fork 500
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
fix: respond to media query change events when theme is auto
#1731
Conversation
🦋 Changeset detectedLatest commit: 19d163f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Hello! Thank you for opening your first PR to Starlight! ✨ Here’s what will happen next:
|
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.
Thank you for this PR @techfg! I made a couple of small tweaks — mainly to use the existing #loadTheme()
method instead of writing additional logic for extracting the theme from the select menu.
There is a small “bug” in this version of things because we actually have two instances of <ThemeSelect>
in most pages — one in the nav bar, one in the mobile menu. That means we have two media query event listeners that, due to some internal wiring, each update both instances. I’ll tackle that separately though as it will need a small refactor to pull some of this logic up out of the custom element class.
Appreciate you helping out 🙌
@delucis - Thanks for reviewing so quickly and getting merged! Your changes make sense, much simpler, and yeah, noticed the multiple |
Description
auto
theme does not respond to change in system/browser preferences #1684Will respond to media change query events and update the theme to system/browser preference when current theme is set to
auto
Technically it's a visual change but nothing visual about the package changed :)
I didn't see any existing tests in/around the components, specifically
ThemeSelect
. If I overlooked and/or you'd like me to add some, just let me know.