-
Notifications
You must be signed in to change notification settings - Fork 214
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
Feature/382 refactor theme panel component #393
Feature/382 refactor theme panel component #393
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
/** A hotkey to quickly show/hide the panel. | ||
* Set to `null` or `false` to disable the hotkey. | ||
* @default "T" | ||
*/ | ||
openHotkey?: string | null | false; | ||
/** A hotkey to quickly toggle the appearance. | ||
* Set to `null` or `false` to disable the hotkey. | ||
* @default "D" | ||
* */ | ||
toogleAppearanceHotkey?: string | null | false; |
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.
We wouldn't want to expose a way to customize the shortcut itself – the theme panel should always work the same in different places. Having different shortcuts on different websites is going to feel unpredictable.
Just a boolean prop to disable the shortcuts should be fine, e.g. disableShortcuts
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 respectfully disagree with your perspective. Default behavior can indeed remain consistent, but offering customization options allows users to adapt the component to fit seamlessly into their workflows. If a user chooses to change a defined hotkey, it should be within their responsibility, and providing such flexibility enhances the usability and adaptability of the library.
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.
By the way, just noticed that Radix Primitives has an example of Custom hotkey prop for Toast component 🙂
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.
FWIW Radix Primitives is designed to be more flexible, where Themes is a more opinionated starter. I could be convinced on this, however. I agree it can be annoying to deal with potential conflicts.
Hi! Just wanted to weigh in here since we've had a little maintainer shuffle since this PR was opened. Any chance we could break this into two separate, stacked PRs? I'd like to consider each new feature independently since there's a little unresolved feedback. Specifically it'd be nice to see it broken into three:
I know it's been a while, so totally understand if you've moved on. Happy to take this as a feature request and re-implement if we decide to move forward. |
Description
Main changes:
open
andonOpenChange
props to theThemePanel
component using theuseControllableState
hookThemePanel
andThemePanelImpl
components. Remove redundant interfacesuseHotKey
hook (no external dependencies)openHotkey
andtoogleAppearanceHotkey
propsnull
orfalse
to disable a hotkeyopenHotkey
is nullThings to improve in the future:
Control + F
orShift + H
useHotKey
implementation is very barebones. Consider using a library like react-hotkeys-hookTesting steps
Use cases:
Relates issues / PRs
Refs #382