Add cmd-k/ctrl-k shortcut to focus searchbar #1870
Conversation
assets/js/search-bar.js
Outdated
| } else { | ||
| searchInput.setAttribute('placeholder', 'Search - ctrl K') | ||
| } | ||
| } |
There was a problem hiding this comment.
We should not change the placeholder. Let's keep it as it is right now. I think / is still the most convenient for most users (and also what GitHub uses).
There was a problem hiding this comment.
Just as one datapoint to the opposite, / is (IMO) very inconvenient on a Swedish keyboard. Looking forward to using ctrl-k in ex_doc!
There was a problem hiding this comment.
Oh, good point. You can also press s. Maybe we should change the placeholder to say press s instead? 🤔
There was a problem hiding this comment.
No problem-o! I forgot about github as for some reason it's one of the only sites where I don't use its keyboard shortcuts. As a Vim user I do find / an odd choice and since it's used for search within a document whereas k is used to search the docs, but github is obviously good precedent! In any event I'm happy to bring back the placeholder. Let me know if you'd rather I replace it with s.
There was a problem hiding this comment.
Let's only remove the placeholder changes for now, we can revisit in another PR.
sodapopcan
force-pushed
the
search-bar-shortcut
branch
3 times, most recently
from
609e4be
to
c4ea7c4
Compare
sodapopcan
force-pushed
the
search-bar-shortcut
branch
from
c4ea7c4
to
74af926
Compare
|
💚 💙 💜 💛 ❤️ |
Hello ex_doc team!
This PR adds
cmd-k(Mac) andctrl-k(Win, Linux) to focus the search input.This is the first of two features I agreed to take on discussed in #1860. The second part is prevent the entire page from scrolling to the top when focusing the search bar and instead making it slide down over the content. I'm happy to roll that into this PR but I wanted to get a review on this now since I don't have easy access to Windows of Linux machines so I would appreciate if someone could test the
ctrl-kversion.I tried to stay very low-ceremony with this PR doing the simplest thing I could think of. I've added a simple
hasModifieroption for shortcuts which will require that the system-appropriate shortcut key be used. While of course/still works, I've made the placeholder hint show theKversions instead since I feel that is far more common these days. Because it's system dependent, this had to be done with JavaScript.Thanks, and the resolution to #1860 is coming soon!