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.

Sign up for GitHub

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

Jump to bottom

use event.key for search shortcut #1525

Merged
merged 3 commits into from
Oct 19, 2023
Merged

use event.key for search shortcut #1525

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
084e979
use event.key for search shortcut
gabalafou Oct 18, 2023
4f12573
suggestions from review
gabalafou Oct 19, 2023
c0d2808
caps lock
gabalafou Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/pydata_sphinx_theme/assets/scripts/pydata-sphinx-theme.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,12 +236,18 @@ var addEventListenerForSearchKeyboard = () => {
(event) => {
let input = findSearchInput();
// toggle on Ctrl+k or ⌘+k
if ((event.ctrlKey || event.metaKey) && event.code == "KeyK") {
if (
(event.ctrlKey || event.metaKey) &&
(/k/i.test(event.key) || event.keyCode === 75)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this might seem obscure for folks unfamiliar with non-QWERTY layouts, perhaps a comment like this one would be good

Suggested change
(/k/i.test(event.key) || event.keyCode === 75)
// Needed for DVORAK key-mapping
(/k/i.test(event.key) || event.keyCode === 75)

Copy link
Collaborator

Choose a reason for hiding this comment

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

why not simply "non en-iso" instead of "DVORAK" ? If I understand correctly this fix is for any keyboard mapping right ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think it really needs commenting. Most keyboard shortcuts should match the letter (event.key) that would appear in a text editor or input field and not the physical key (event.code) on your keyboard. I know, I know, the terminology is confusing, but that's the way it is.

The exception would be if you're really trying to match the shortcut to a specific physical place on the keyboard. For example, imagine if you were trying to create an app that turns your keyboard into a piano-like musical instrument. Then you would want to map the physical location of keys on the keyboard rather than the letters.

I will add, however, that it's a little fuzzy to me what should happen when apps are internationalized. In a French computer, for example, I think ctrl + t opens a new browser tab, even though the word for tab in French, which is "onglet", does not begin with a "t". But it's weird to me that keyboard shortcuts that were chosen because of a first-letter mnemonic in English (for example: ctrl+s because Save begins with the letter "s") should stay the same across languages.

But the bottom line in our case is that if we are showing the keyboard shortcut in the search bar across all languages as ctrl + k then we should bind to the letter, not the key.

Read event.code and event.key for more explanation.

I think leaving the code without a comment will actually be less confusing than adding a somewhat cryptic comment about key-mapping because standard practice is to use event.key for most keyboard shortcuts.

) {
event.preventDefault();
toggleSearchField();
}
// also allow Escape key to hide (but not show) the dynamic search field
else if (document.activeElement === input && event.code == "Escape") {
else if (
document.activeElement === input &&
(/escape/i.test(event.key) || event.keyCode === 27)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above?

) {
toggleSearchField();
}
},
Expand Down