-
Notifications
You must be signed in to change notification settings - Fork 56
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
Add highlight for Fn key in case F1-F12 is pressed. Fixes #29 #137
Conversation
Fixes #29 |
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.
This is great, thank you! What do you think, @vegetabill ?
My only concern is that fn
continues to have focus after the fn1-12
keys are released, up until the reset for the next question. Could you have it highlight and un-highlight at the same time as the fn1-12
key?
Done! Could you please review it again? |
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.
This looks great! I just requested one required and one optional revision. Thanks for working on this, @Karska-dev
public/scripts/shortcuts.json
Outdated
{ | ||
"answer": "F2", | ||
"question": "How do you rename a symbol in VSCode?", | ||
"keys": [113], | ||
"shortcutType": "mac vscode" | ||
}, |
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.
Can you move this to the end of the file just so it won't mess up the existing numbering? (and because it's a fairly advanced shortcut compared to the others at the start)
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.
Done!
public/scripts/main.js
Outdated
$("#"+e.key.toLowerCase()).addClass("pressed"); | ||
$("#fnc").addClass("pressed"); |
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.
For all of the CSS manipulations, we have an open PR, #135, which is replacing the jQuery with the native DOM functions. If you don't mind, it would be better to use those for these new lines, otherwise it will be inconsistent when both are merged later.
The native, non-jQuery code looks like this:
document.querySelector("#fnc").classList.add("pressed")
If it's too much trouble, though, you can leave it like this and it can be fixed later.
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.
Done! Can you please review one more time?
…SCode shortcut to the end.
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.
Thanks for taking the extra effort on the CSS selectors.
According to all mentioned above links, and especially this one - https://superuser.com/a/432622, OS do not see specific scancode for Fn key. It's only possible to see the scancode of combination Fn and F1-F12.
So, there is no possibility to highlight the Fn key separately while it pressed.
As a possible solution, I've added highlighting of the Fn key if any of F1-F12 codes are received(=Fn+F1-12 are already pressed together.)
Also, I've added Fn highlighting to hints.
For testing purposes I've added "How do you rename a symbol in VSCode?" shortcut to shortcut.json.
The only issue I have with F11 on MacOS - it can't be received by Browser, on Windows works well.