Skip to content
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

Merged

Conversation

viniciusteixeiradias
Copy link
Contributor

@viniciusteixeiradias viniciusteixeiradias commented May 1, 2023

…alize

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Provide a description in this PR that addresses what the PR is solving. If this PR is going to solve an existing issue, please reference the issue (e.g. close #123).

Solving this problem: vuejs/docs#2240.

What is the purpose of this pull request?

  • Bug fix
  • New feature
  • Documentation update
  • Other

Description

Previously, the initializate method was called in onMounted Docsearch.ts, but there was a change that removed it and the method responsible for triggering docsearch before executing the initializate became useDocsearchHotkeyListener, this method defines a keydown eventLintener when mounting the component.

My change was to add the '/' key to the useDocsearchHotkeyListener validation.

@Mister-Hope
Copy link
Member

Docsearch only support command + k. Adding / is confusing.

@Mister-Hope Mister-Hope closed this May 1, 2023
@viniciusteixeiradias
Copy link
Contributor Author

viniciusteixeiradias commented May 1, 2023

Docsearch only support command + k. Adding / is confusing.

@Mister-Hope Confused? What do you mean? The / works in Docsearch, the problem happens on the first load.

@Mister-Hope Mister-Hope reopened this May 1, 2023
@Mister-Hope
Copy link
Member

Didn't know that, I will test that later

@viniciusteixeiradias
Copy link
Contributor Author

@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.

@Mister-Hope
Copy link
Member

Mister-Hope commented May 2, 2023

Hi there, holiday during April 29th - May 3rd in Chinese, will be back later.

Comment on lines +8 to +17
const isHotKeyBind = event.key === 'k' && (event.ctrlKey || event.metaKey)
const isSlashKey = event.key === '/'

if (!isSlashKey && !isHotKeyBind) {
return
}

event.preventDefault()
callback()
remove()
Copy link
Member

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

Suggested change
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()
}

Copy link
Contributor Author

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.

@meteorlxy
Copy link
Member

Also didn't know that slash is the default hot key of docsearch. Nice catch!

@meteorlxy meteorlxy merged commit 3382bb1 into vuepress:main May 8, 2023
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants