-
Notifications
You must be signed in to change notification settings - Fork 273
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
feat: add defaultMode
to DarkMode
component
#1384
base: main
Are you sure you want to change the base?
Conversation
@ttytm is attempting to deploy a commit to the Themesberg Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughA new exported variable Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/lib/darkmode/DarkMode.svelte (2 hunks)
Additional comments not posted (3)
src/lib/darkmode/DarkMode.svelte (3)
7-7
: LGTM!The addition of the
defaultMode
variable is correct and aligns with the PR objective.
29-30
: LGTM! But verify the logic.The logic to check for
defaultMode
and apply the appropriate theme appears correct.However, ensure that this logic works as intended in all scenarios.
32-32
: LGTM!The logic to handle browser preference if no explicit or default mode is set is correct.
You can set the initial theme. |
Thanks for the response. Checking the code, the module adds the
So as far as I can see test: the suggestion doesn't fully work. It's not possible to set a dedicated default light mode. Because if the browser is set to dark mode it will use dark as default, even when This means the default mode is not light as the documentation states, but it is based on the browser setting. But the goal is to allow to set a default mode explicitly and fall back to the browser setting if no default mode is passed. For a reproduction: open a site where |
This comment was marked as off-topic.
This comment was marked as off-topic.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
DarkMode has unused export property 'defaultMode'. If it is for external reference only, please consider using |
Thanks for the review. I'm gonna update accordingly soon. |
📑 Description
Adds the ability to specify a defaultMode.
I was missing the easy ability to set a default mode for first-time-visitors.
As a reference: I was used to working with the package
mode-watcher
, it allows to set a default mode.It makes workaround-overrides/boilerplate like below obsolete:
With the changes one could just:
Status
The PR is mostly but not fully finished. Please let me know if completing the PR has a chance for a merge @shinokada. Sorry about this approach, I'm trying to balance things due to time constraints.
✅ Checks
ℹ Additional Information