-
-
Notifications
You must be signed in to change notification settings - Fork 332
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 keyboard shortcut command to focus chat input #876
Conversation
If there is still intent to re-use the extracted chat package then hard-coding an ID would be also undesirable for the reason that with CC @brichet in case if you have any thoughts on the API |
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.
@krassowski Thanks! I agree with your direction of using a Lumino signal here instead of the alternatives you identified. I left a minor naming suggestion and a question below.
Can you verify that the proposed key combo Accel Shift 1
isn't bound by any other command native to JupyterLab? Not sure how JupyterLab handles conflicts, but I think it is best to avoid them.
The use of 1 as a shortcut key may be an issue for our French keyboard users, as on a common AZERTY layout, this is a shifted character; the key by itself produces &, and SHIFT+& produces 1. I would recommend testing this with an AZERTY keyboard, if possible. |
Thanks @krassowski for pinging me and updating #862. Indeed, there is still intent to use the extracted |
3b203d0
to
fb210c5
Compare
I do not see why this would be an issue - the French AZERTY is no different with this respect from US international (which produces Access to the blue keys could be problematic if we've chosen Ctrl + Shift + [2-9 or 0]. To be on the safe side though, I pushed a commit which ensures that this shortcut will never interfere with any key producing a character on any keyword: fb210c5. Still, I would welcome any better suggestions (but if we do not have any, let's ship and be open to user feedback on choosing a better default later). |
Yes, it is not. I also tested that this shortcut is not used by Chrome, Firefox nor GNOME Web. |
I don't have a physical AZERTY keyboard with me, but I set my OS to treat my US keyboard as an AZERTY layout, and I pressed what, in the US, are the number keys. The 1 key (US) produced &. The other keys caused the black characters in the image above to appear: 2 became é, for example. |
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 everybody for providing feedback on this PR! Here's my thoughts on this:
- We should keep this shortcut undocumented until we a) do more testing (e.g. to test @JasonWeill's concerns about AZERTY layouts) and b) verify that this key combo doesn't conflict cause excessive conflicts with other JupyterLab extensions.
- We will not consider a change to this shortcut a breaking change until it is explicitly documented in the ReadTheDocs page.
Keeping these 2 points in mind, I feel comfortable merging this PR. @krassowski Thank you for contributing this, and for all of your hard work recently!
fb210c5
to
dedfe42
Compare
I tested it with an AZERTY keyboard and it works fine with |
* Focus chat input with keyboard * Focus chat input with keyboard * Rename `inputFocusRequested` to `focusInputSignal` * Make sure that the shortcut does not interrupt default action
This is one way to fix #799
This implementation uses a signal to trigger the focus event.
There are two alternatives:
inputRef
from where the command is defined, all the way down to the React elementA distinct advantage of the alternative approaches would be that they could also check if the input element is already focused (something that a signal cannot do). That said, using a signal is clearer with respect to the API surface, and the detection of focus state can be done by using a selector on the shortcut.
If you would prefer me to use one of the alternative approaches I would argue in favour of
inputRef
over hard-coded ID on the basis of better type safety and integrity guarantees.--
The first commit includes a shortcut
Accel Shift 1
but I am not set on keeping this shortcut, we could choose a different on in the review, or remove it and let user choose a shortcut (but this would mean users need to define it in the JSON Settings Editor manually as the UI editor does not support it yet, c-f jupyterlab/jupyterlab#13705).To enable loading of the shortcut from schema this PR includes a change to the plugin ID, but I also opened a dedicated PR to fix this across the board: #872.
Please let me know what you all think.