Skip to content

feat: Implement shortcut to focus search #275

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

Merged
merged 2 commits into from
Mar 30, 2021

Conversation

dmtrKovalenko
Copy link
Contributor

"/" to focus search is a widely popular shortcut for the quick focus of the search input.

Examples:

  1. Github (take a look on the top-left corner)
  2. https://tailwindcss.com/
  3. https://docs.cypress.io/
  4. even https://reasonml.github.io/

There is another one – meta+k I can implement it as well if you want :)

let inputRef = React.useRef(Js.Nullable.null)
let (state, setState) = React.useState(_ => Inactive)
let handleKeyDown = e => {
if e.key == "/" {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this may not work as expected in IE, but who cares?

@ryyppy
Copy link
Member

ryyppy commented Mar 28, 2021

Hey, thanks for the idea.
I am not sure we should implement this right now, because it might have unexpected side-effects on different parts of the website.

For instance, right now it breaks the playground since you can't use the / character in the editor.

@dmtrKovalenko
Copy link
Contributor Author

dmtrKovalenko commented Mar 28, 2021

Thanks, @ryyppy for the fast feedback, but this is a kinda important feature (at least for the home page). If you are not sure about sopping event propagation on the text-fields what do you think about making this only for the home page?

I am a little bit suffering right now with rescript docs, cause I need to take mouse all the time :)

@dmtrKovalenko
Copy link
Contributor Author

Or I have another idea of how to make it more correct – what if we will handle input only if the event target is not an input? I think this will handle all the edge cases, maybe except content-editable divs. And probably will require some instanceof hack for rescript

@ryyppy
Copy link
Member

ryyppy commented Mar 28, 2021

@dmtrKovalenko would be interesting to see how e.g. tailwind's docs implemented that feature. I think it is not enough just making assumptions on the dom element type.

@dmtrKovalenko
Copy link
Contributor Author

I was doing this feature for cypress docs, but ok I'll make some kind of research of how this done ☑️

@dmtrKovalenko
Copy link
Contributor Author

For tailwindcss.com – https://github.com/tailwindlabs/tailwindcss.com/blob/0ce320045d2836ea6fc68ccb834c93364536f731/src/components/Search.js

They are using @docsearch/react. I didn't find sources anywhere O_O but if you will install npm package you will see

import React from 'react';
export function useDocSearchKeyboardEvents(_ref) {
  var isOpen = _ref.isOpen,
      onOpen = _ref.onOpen,
      onClose = _ref.onClose;
  React.useEffect(function () {
    function onKeyDown(event) {
      if (event.keyCode === 27 && isOpen || event.key === 'k' && (event.metaKey || event.ctrlKey)) {
        event.preventDefault();

        if (isOpen) {
          onClose();
        } else {
          onOpen();
        }
      }
    }

    window.addEventListener('keydown', onKeyDown);
    return function () {
      window.removeEventListener('keydown', onKeyDown);
    };
  }, [isOpen, onOpen, onClose]);
}

But this will steal focus from any keyboard node, much better example is material-ui which is doing exactly what I said – checks that the current active target is not a focusable element

https://github.com/mui-org/material-ui/blob/0e00a4997700643188a43d1636ba8a71d333209a/docs/src/modules/components/AppSearch.js#L162

      const matchMainShortcut =
        (nativeEvent.ctrlKey || nativeEvent.metaKey) && nativeEvent.key === 'k';
      const matchNonkeyboardNode =
        ['INPUT', 'SELECT', 'TEXTAREA'].indexOf(document.activeElement.tagName) === -1 &&
        !document.activeElement.isContentEditable;

      if (matchMainShortcut && matchNonkeyboardNode) {
        nativeEvent.preventDefault();
        inputRef.current.focus();
      }

@dmtrKovalenko
Copy link
Contributor Author

@ryyppy I updated my implementation to avoid any kind of focus stealing

type keyboardEventLike = {key: string, ctrlKey: bool, metaKey: bool}

@bs.val @bs.scope("window")
external addKeyboardEventListener: (string, keyboardEventLike => unit) => unit = "addEventListener"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, not sure if you are aware of this. You can set a fixed param like keydown via the as("keydown") _ placeholder:

Suggested change
external addKeyboardEventListener: (string, keyboardEventLike => unit) => unit = "addEventListener"
external addKeydownEventListener: (@as("keydown") _, keyboardEventLike => unit) => unit = "addEventListener"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will merge anyways, since this will be easy to refactor later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice didn't know this is possible! :)

@ryyppy
Copy link
Member

ryyppy commented Mar 30, 2021

Looking good! Thanks for going the extra mile double checking with the other implementations.

@ryyppy ryyppy merged commit 70bdf2b into rescript-lang:master Mar 30, 2021
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.

2 participants