-
Notifications
You must be signed in to change notification settings - Fork 5
feat: keyboard support for input area #41
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
base: main
Are you sure you want to change the base?
feat: keyboard support for input area #41
Conversation
| "serve-static": "^1.15.0" | ||
| }, | ||
| "optionalDependencies": { | ||
| "faiss-node": "^0.5.1" |
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.
Update the README to indicate the RAG support is optional. Can you also document the npm install commands for enabling or skipping this functionality.
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.
@cindyli I added some information in the README. Please let me know if it makes sense or if you'd like further changes.
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.
The update on README looks good to me. Thanks, @jobara.
…ncy and requiring CMake
klown
left a comment
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 this, @jobara. It looks good!
In addition to the minor changes that I have suggested, there are a couple of items that you could help with:
-
The
README.mdhas a spelling mistake on line 61 -- not your fault. It is "quried" instead of "queried". If you could fix it here in this PR, that would be great. -
The focus ring on the input area feels subtle to me (tested on FF and Safari), but that's a UI design issue and has nothing to do with keyboard navigation per se. However, feel free to suggest changes to
ContentBmwEncoding.scssto improve the focus style.
Thanks again.
src/client/ContentBmwEncoding.ts
Outdated
| }); | ||
| } | ||
|
|
||
| export function moveCursor (positionChange = 1) { |
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.
Provide the type of positionChange (number).
| import "@testing-library/jest-dom"; | ||
| import { html } from "htm/preact"; | ||
| import { ContentBmwEncoding } from "./ContentBmwEncoding"; | ||
| import { ContentBmwEncoding, } from "./ContentBmwEncoding"; |
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.
Extraneous trailing comma after ContentBmwEncoding.
src/client/ContentBmwEncoding.ts
Outdated
| } | ||
|
|
||
| export function moveCursor (positionChange = 1) { | ||
| positionChange = Math.round(positionChange); |
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.
While I think the use of Math.round() here is a good idea in general, I'm not sure it's necessary in this case.
First, the moveCursor() function is only used within this file, by incrementCursor() and decrementCursor(). It need not be exported.
Secondly, since the calling functions always pass 1 or -1, is it necessary to round the argument?
If it were exported because of broader use, then rounding is useful.
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.
Are you saying that moveCursor also shouldn't be imported? I have it exported in case you may want to use it later, maybe with something like home and end functions, or maybe there will be some need to jump to different parts of a phrase.
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'm saying that moveCursor() is not in fact imported anywhere now. My rule of thumb is to not export functions/data when exporting is unnecessary. If it turns out later that an export is needed, then it can be done at that point, along with the changes that made it a requirement.
Regarding home, end, and other related potential functions, they might be defined here locally the way incrementCursor() and decrementCursor() are. If so, moveCursor() would not need to be exported in that case either.
But you are also giving me ideas. Would you like to extend this PR to include the home and end keystrokes? They strike me as useful.
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've added keyboard handlers for home and end.
| * @param {number} max - The maximum value to be returned | ||
| * @returns {number} - The constrained value | ||
| */ | ||
| function clamp (value: number, min: number, max: number) { |
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.
Just a note -- no change necessary. I thought that a clamp() function might already exist as a built-in within TypeScript or JavaScript, and asked duck.ai if there was one. Its reply was that there was none, and then provided the code. It was the same as yours, but without the type declarations. So, your implementation must be right :-)
@klown I think we should talk about this a bit more. Perhaps I can look into it as part of a future PR. I need to understand more about the current colour scheme and layout I think. |
| if ( | ||
| event.key === "Home" || | ||
| (event.ctrlKey && event.key === "a") || | ||
| (isApplePlatform && event.metaKey && event.key === "ArrowLeft") | ||
| ) { | ||
| event.preventDefault(); | ||
| moveCursorToHome(); | ||
| } | ||
|
|
||
| if ( | ||
| event.key === "End" || | ||
| (event.ctrlKey && event.key === "e") || | ||
| (isApplePlatform && event.metaKey && event.key === "ArrowRight") | ||
| ) { | ||
| event.preventDefault(); | ||
| moveCursorToEnd(); | ||
| } |
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.
The key commands supported are:
- Home
- Home
- fn+left arrow
- ctrl+a
- command+left arrow (Apple only)
- End
- End
- fn+right arrow
- ctrl+e
- command+right arrow (Apple only)
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.
As I said in the meeting today, this is quite thorough, even including the emacs keystrokes for beginning and end. One slight worry: Home/End mean "Top of the document"/"Bottom of the document", while ctrl-a/ctrl-e mean "beginning of the current line/end of the current line". It's not a problem in the current context since "Top of the document" and "beginning of the current line" are the same thing. If the input ever becomes multi-line like a text-area element, then this will need to be revisited.
| export function moveCursorToHome () { | ||
| moveCursor(Number.NEGATIVE_INFINITY); | ||
| }; | ||
|
|
||
| export function moveCursorToEnd () { | ||
| moveCursor(Number.POSITIVE_INFINITY); | ||
| }; |
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've exported these in case you wanted to create a button on the UI. But I could also just remove the functions and make a direct call to moveCursor in the handler.
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.
Also I'm using INFINITY here so that I don't have to make calculations based on changeEncodingContents.value, but that would be another option.
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.
Yes, there definitely should be buttons for Home/End so the exports are good.
I think that using INFINITY makes it easier than doing calculations using the changeEncodingContents signal, especially since it ultimately is processed by clamp() anyway. And, it's clear what it means ("all the way to the left" and "all the way to right").
|
Thanks @jobara. I have run into an issue and am trying to decide how to handle it. Here's the background: I added a global key handler for navigating back to a previous palette, with the idea that users could access it at any time, no matter what was focused. However, the actual keystroke is the back-tick (`) which can be a legitimate character in a text input. Thus, there is code to check whether keyboard focus is in any kind of text input control so that the back-tick can be added to that text. Since the symbol input area has a role of "textbox", once it has focus, the "navigate back" keystroke stops working. It probably shouldn't. I can see a fix where if focus is in the symbol input area, then a back-tick keystroke is not ignored. I could make a branch from your branch and add the one or two lines to accomplish that. Or you could look into it. What do you think? |
|
@jobara I found another issue. When the "Backward" and "Forward" buttons are pressed, the cursor moves backward or forward, and there is a voice output, "backward" or "forward", respectively. When the arrows keys are used to move the cursor, it is silent. This is inconsistent. |
|
@jobara a followup to one of my earlier comments:
I've created the branch and made the fix. Should I go ahead and make a PR for you to review? |
Sure that would be great. However, I probably won't have time to look at it today. |
I have added this now, and also announcing when you use home and end. I think the implementation may need to be revisited though if we want to have more support for disabling some or all off the speaking. |
faiss-nodeto be an optional dependencyclampfunction