-
Notifications
You must be signed in to change notification settings - Fork 388
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(v3): allow overriding default toggle key #1196
base: main
Are you sure you want to change the base?
Conversation
✔️ Deploy Preview for docsearch ready! 🔨 Explore the source changes: 23475f9 🔍 Inspect the deploy log: https://app.netlify.com/sites/docsearch/deploys/61a4ebf4e9ff330008631128 😎 Browse the preview: https://deploy-preview-1196--docsearch.netlify.app |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 23475f9:
|
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.
This is the kind of feature that we want to be opinionated about, and not create disparities in the DocSearch ecosystem. Users can already use le lower-level components to customize the keyboard shortcuts.
It's clearly the reason why I wasn't sure about adding it to the options, but it's requested quite frequently so it's better to have a public discussion. On the other hand, I still think it's good to provide a bit of customization without having to re-implement the component. |
We don't necessarily want to please everybody in all situations. The ⌘K pattern was fully studied and introduced with a clear plan in mind: democratize a mental pattern for developers when using docs. We don't want to send blurry signals and have developers wonder if hitting ⌘K would start searching or not. Our take is that it should. We shouldn't make it an option just because we can – options are the main path to confusion. DX is also about constraints. If we don't recommend customizing the keyboard shortcut, we shouldn't make it easy to do so. |
I did not had strong opinions on this, but I'll share what's top on my mind.
My 2 cents on this. |
Agree with Francois, low level customization should be enough in that case. |
I agree and this is why I wanted to have this open to a possible debate.
This is exactly why I first doubted when opening this PR, but we have developers with different background using our tool. While providing the best DX (by default in that case) is obviously our concern, we should also ease their journey when they want to move out of our constraints.
I'd also add that almost half of our users are using website generators and only see DocSearch components through their website config file, providing an option at this level is also a great DX improvement. |
If we all have different opinions we need facts:
|
Summary
This PR adds a new option to the
DocSearch
andDocSearchModal
components to override the defaulttoggleKey
(k) shortcut.Motivations
While cmd+k usage slowly spread, we can understand that users would like to use something else (e.g. cmd+f) without having to reimplement the whole component.