-
Notifications
You must be signed in to change notification settings - Fork 14
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: up/down arrow navigation now only triggers when cursor is at the start or end of multiline inputs #208
Conversation
…e start or end of multiline inputs
Personal preference/nit : From a UX perspective, I preferred having the arrows solely for prompt navigation, making it much faster to navigate through prompts. If the user wants to navigate (and modify) the input, it can already do so by moving the cursor with the mouse or using the left/right arrows. Have we validated this decision with UX? |
// Check if cursor is on the first line | ||
const isFirstLine = !textBeforeCursor.includes('\n'); | ||
|
||
// Check if cursor is on the last line | ||
const isLastLine = !textAfterCursor.includes('\n'); |
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 to double check, is it safe to deduce first/last line by checking for \n
? i am thinking of some edge cases with input (maybe being copy pasted) starting/ending with empty lines
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 did some more testing, and I think it is not a safe deduction to check for '\n'
to deduce first/last line, you can write on multiple lines without using new lines (\n). To fix this we might need to check the height of the input field or count the n of lines somehow, something roughly in this style, even though it looks a bit hacky and it might be tricky in certain edge cases :
// get the height of the prompt text input
const promptTextInputHeight = this.promptTextInput.render.offsetHeight;
// avg line height
const avgLineHeight = 20;
// derive an estimate avg n of lines
const estimatedLines = Math.ceil(promptTextInputHeight / avgLineHeight);
// count n of chars
const inputTextNofChars = inputText.length;
// derive an estimate avg n of chars per line
const avgCharsPerLine = Math.ceil(inputTextNofChars / estimatedLines);
// derive line in which we are based on cursorPosition and avgLines
const line = Math.ceil(cursorPosition / avgCharsPerLine);
// define isFirstLine
const isFirstLine = line === 0;
// define isLastLine
const isLastLine = line === estimatedLines - 1;
Personal IMO: using mouse breaks "typing" flow and maybe more jarring experience in non-terminal visual UI mode. Given this is multi-line input field, it is not as intuitive UI, as I expect to move cursor when I type. Maybe having special short cut to history jump would be more intuitive |
I've been thinking about this as well. A key combination for history jumps might be better. Will get back to this when I'm back in office on Thursday. |
For users whose flow does not involve using a mouse, it's annoying to have to use a mouse to edit the prompt. Also, how often does a user want to scrollback through their prompt history? If they need to go far, they can just scroll up in the chat... and I wonder how rare that is relative to needing to edit the prompt |
Problem
If there is multiline text in prompt field, it is not possible to navigate to upper lines by using app arrow, it triggers historical navigation.
Solution
When there is text in the prompt field which is more than 1 line, until user approaches to the first line, keep the default behavior of the arrow keys.
Tests
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.