-
Notifications
You must be signed in to change notification settings - Fork 923
fix(plugin-docsearch): open docsearch with slash key before run initi… #1323
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
fix(plugin-docsearch): open docsearch with slash key before run initi… #1323
Conversation
Docsearch only support command + k. Adding / is confusing. |
@Mister-Hope Confused? What do you mean? The |
Didn't know that, I will test that later |
@Mister-Hope Maybe we need to add this validation below: const isEditingContent = (e: KeyboardEvent): boolean => {
const element = e.target as HTMLElement;
const tagName = element.tagName;
return (
element.isContentEditable ||
tagName === 'INPUT' ||
tagName === 'SELECT' ||
tagName === 'TEXTAREA'
);
} I am still awaiting your feedback to keep working on this issue. |
Hi there, holiday during April 29th - May 3rd in Chinese, will be back later. |
const isHotKeyBind = event.key === 'k' && (event.ctrlKey || event.metaKey) | ||
const isSlashKey = event.key === '/' | ||
|
||
if (!isSlashKey && !isHotKeyBind) { | ||
return | ||
} | ||
|
||
event.preventDefault() | ||
callback() | ||
remove() |
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.
BTW, I do not like the changed here, reversing the logic doesn't make sense, the handler is catching some keys here, so a positive judgement should be better
const isHotKeyBind = event.key === 'k' && (event.ctrlKey || event.metaKey) | |
const isSlashKey = event.key === '/' | |
if (!isSlashKey && !isHotKeyBind) { | |
return | |
} | |
event.preventDefault() | |
callback() | |
remove() | |
const isHotKeyBind = (event.key === 'k' && (event.ctrlKey || event.metaKey)) | |
const isSlashKey = event.key === '/' | |
if (isSlashKey || isHotKeyBind) { | |
event.preventDefault() | |
callback() | |
remove() | |
} |
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 understand your point, but the "early return" is a very considerable and advantageous approach to make the logic clearer and more evident (indicated when talking about clean code). This function will not do anything else if the hotkeys are not triggered, so this way avoids the need to put the entire logic of the method inside the if.
Also didn't know that slash is the default hot key of docsearch. Nice catch! |
…alize
Before submitting the PR, please make sure you do the following
close #123
).Solving this problem: vuejs/docs#2240.
What is the purpose of this pull request?
Description
Previously, the
initializate
method was called in onMountedDocsearch.ts
, but there was a change that removed it and the method responsible for triggeringdocsearch
before executing the initializate becameuseDocsearchHotkeyListener
, this method defines akeydown
eventLintener when mounting the component.My change was to add the '/' key to the
useDocsearchHotkeyListener
validation.