Skip to content
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

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

Jurredr
Copy link
Contributor

@Jurredr Jurredr commented Dec 16, 2024

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

  • I have tested this change on VSCode
  • I have tested this change on JetBrains

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Jurredr Jurredr added the improvement Improvement of an existing feature label Dec 16, 2024
@Jurredr Jurredr self-assigned this Dec 16, 2024
@Jurredr Jurredr requested a review from a team as a code owner December 16, 2024 15:31
@francescoopiccoli
Copy link
Contributor

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?

Comment on lines +333 to +337
// 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');
Copy link
Contributor

@francescoopiccoli francescoopiccoli Dec 16, 2024

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

Copy link
Contributor

@francescoopiccoli francescoopiccoli Dec 18, 2024

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;

@viktorsaws
Copy link

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?

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

@Jurredr
Copy link
Contributor Author

Jurredr commented Dec 16, 2024

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?

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.

@bergjaak
Copy link

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?

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

@Jurredr Jurredr merged commit 6f71bb3 into main Dec 20, 2024
2 checks passed
@Jurredr Jurredr deleted the jurredr/prompt-arrow-navigation-on-multiline branch December 20, 2024 09:05
@Jurredr Jurredr restored the jurredr/prompt-arrow-navigation-on-multiline branch December 20, 2024 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants