fix(#1502): add theme prop to sonner - #1694
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds automatic color-mode detection to the Sonner toaster, maps ChangesSonner theme support
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: private package registry requires authentication. Disable ESLint in CodeRabbit settings or use public packages. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/v4/registry/new-york-v4/ui/sonner/Sonner.vue`:
- Around line 30-31: The current template spreads v-bind="props" then
unconditionally sets :theme="theme", which in Vue 3 causes the computed theme to
always override any consumer-supplied props.theme; update the binding so the
component uses the consumer-provided theme when present by changing the
computed/prop usage to use props.theme ?? <computed fallback> (or adjust the
computedTheme getter) and then bind :theme to that result (or replace
:theme="theme" with :theme="props.theme ?? themeFallback"); target the
v-bind="props" usage and the computed/theme variable in Sonner.vue and ensure
ToasterProps.theme is respected.
- Around line 3-11: The file calls useColorMode but doesn't import it; add
useColorMode to the import list from its source (e.g., import { useColorMode }
from "@vueuse/core" or from your project's composables) so the reference inside
Sonner.vue (where useColorMode() is invoked) resolves at runtime; update the
existing import block that currently imports computed to include the correct
useColorMode import instead of leaving it undefined.
|
This is my first OSS contribution, sorry if this might sound stupid but are we supposed to import from |
|
I don't think this should be part of the shadcn library. I think it is best to resolve / manage the color mode outside of this component and simply pass it as |
where did you find that? I searched the code base and found |
I had made a mistake in understanding it. The current implementation is correct right? |
|
Thanks for the PR! The direction is good, but a couple of changes are needed before this can work:
Also worth noting: place |
I've applied these changes in the latest commit |
🔗 Linked issue
Fixes #1502
❓ Type of change
📚 Description
I have added a theme prop to the Sonner component to allow it to work better with dark themes. Resolves #1502
📝 Checklist
Summary by CodeRabbit