-
Notifications
You must be signed in to change notification settings - Fork 26
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
feat: add keyboard support #12
Conversation
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.
Hi @felixk101
Could you change the handler to this
const onKeyPress = (e: KeyboardEvent) => {
const key = e.key.toLowerCase();
if (key === "enter") return submitGuess({ $t: t, guess: userGuess.value });
if (key === "backspace") {
userGuess.value = userGuess.value.slice(0, -1);
return false;
}
if (key.length === 1 && store.availableLetters.includes(key)) {
userGuess.value += key;
return true;
}
};
Also, did you see my other comments?
Just opened this pr in an incognito window and I couldn't see them, so wondering if you got them at all. They basically said the same as above.
@@ -16,10 +25,32 @@ const otherLetters = ref( | |||
); | |||
let userGuess = ref(""); | |||
|
|||
const onKeyPress = (e: KeyboardEvent) => { | |||
if (e.key == "Enter") { |
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 doing this! I probably won't have time to review this properly for a couple days, but a few changes I'd want to see in the meantime are:
- Triple equals for comparison
- Remove console.log
- Guard clauses instead of nested if statements.
If any of those aren't clear, I can give examples once I'm back at my computer (I'm writing this on a phone).
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.
What I mean is something more like this:
const onKeyPress = (e: KeyboardEvent) => {
const key = e.key.toLowerCase();
if (key === "enter") return submitGuess({ $t: t, guess: userGuess.value });
if (key === "backspace") {
userGuess.value = userGuess.value.slice(0, -1);
return false;
}
if (key.length === 1 && store.availableLetters.includes(key)) {
userGuess.value += key;
return true;
}
};
If you just copy the block above into your pr I'd be happy to merge tbh.
Happy to listen to arguments for different approaches either.
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.
I guess these comments only submitted now since I added a new comment through the request changes
dialog 🤦♂️
For the record I'm pretty sure I added the first comment the day you opened the pr, and the second one a few days later. What a pain
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.
It's been a fairly long time now so I've gone ahead and done this myself. Thanks for your contribution though!
Adds keyboard support (Enter, Backspace and Letter Entry)
Closes #9