-
Notifications
You must be signed in to change notification settings - Fork 1
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: Persistent theme settings #347
Conversation
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.
Oh man, I loved that switch in activity bar 😞 It removes the friction to test both themes when developing, can we keep it in activity bar additionally to settings? We could also wrap in dev mode condition if there's a reason we don't want to have it there in prod builds.
Let's make it dev-only for now 👍 |
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! 🙌
src/components/ThemeSwitcher.tsx
Outdated
@@ -4,6 +4,12 @@ import { IconButton, Tooltip } from '@radix-ui/themes' | |||
|
|||
export function ThemeSwitcher() { | |||
const theme = useTheme() | |||
// @ts-expect-error we have commonjs set as module option | |||
const isDev = import.meta.env.DEV |
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.
Would process.env.NODE_ENV === 'development'
work here to prevent @ts-expect-error
?
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.
Thanks for the suggestion. I was under the impression it wasn't available in the render process 👀. Replaced with process.env.NODE_ENV === 'development'
👍
Description
How to Test
Checklist
npm run lint
) and all checks pass.npm test
) and all tests pass.Screenshots (if appropriate):
Related PR(s)/Issue(s)