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

Rework selection inclusion; new Send button UX #905

Merged
merged 13 commits into from
Jul 29, 2024

Conversation

dlqqq
Copy link
Member

@dlqqq dlqqq commented Jul 18, 2024

Description

  • Removes "Include selection" checkbox, and replaces it with an alternative UI
  • Implements a new send button with a dropdown button to the right.
    • When this button is clicked, it opens a menu containing an option with the label "Send message with selection (X lines / 1 active cell selected)".
    • This option is disabled if there is no text selection or active cell.
    • When a text selection is made within the active cell, it takes precedence over the active cell. This allows users to only include a selected range within an active cell.
    • When /fix is the current slash command, "Send message with selection" does the same thing as the default "Send message" button, since both of them should always include a code cell w/ error output when /fix is being called.
  • This feature allows users to include a selection with their prompt just once, instead of having selection inclusion be a feature that is enabled until explicitly disabled.
  • Fixes Remove "Include selection" checkbox #864.

Demo

Screen.Recording.2024-07-23.at.10.49.29.AM.mov

@dlqqq dlqqq added the enhancement New feature or request label Jul 18, 2024
@dlqqq dlqqq marked this pull request as draft July 18, 2024 18:40
@dlqqq dlqqq force-pushed the rework-selection-inclusion branch 2 times, most recently from 254f917 to b2b2395 Compare July 23, 2024 17:56
@dlqqq dlqqq marked this pull request as ready for review July 23, 2024 17:57
@dlqqq dlqqq changed the title WIP: Rework selection inclusion Rework selection inclusion Jul 23, 2024
@dlqqq dlqqq changed the title Rework selection inclusion Rework selection inclusion; new Send button UX Jul 23, 2024
@dlqqq dlqqq force-pushed the rework-selection-inclusion branch from bbbe15e to 917e871 Compare July 23, 2024 18:06
Copy link
Collaborator

@JasonWeill JasonWeill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please update the text and screenshots in the user docs to reflect this new UI?

@dlqqq
Copy link
Member Author

dlqqq commented Jul 23, 2024

Since this is a fairly significant change to the UX, I'm leaving this PR open for a few more days to allow other contributors to review, test, and offer feedback here.

@JasonWeill I'll update the docs after this PR is approved and merged, since the UI may change as others offer feedback.

@jtpio jtpio mentioned this pull request Jul 23, 2024
11 tasks
@JasonWeill
Copy link
Collaborator

The code and UI look great! I noticed that I can use TAB to get to the caret:

image

… but when I press ENTER, instead of opening the menu, the prompt gets sent without the selection. Pressing SPACE causes the menu to appear, though. Not sure if we can override this, and it's a pretty minor annoyance.

@krassowski
Copy link
Member

👍 For context, on GitHub pressing either Enter or Space with focus on the caret does open the menu.

@dlqqq dlqqq force-pushed the rework-selection-inclusion branch from 45671b1 to b983640 Compare July 29, 2024 16:32
@dlqqq
Copy link
Member Author

dlqqq commented Jul 29, 2024

All comments addressed! This PR is ready for another round of review.

@JasonWeill I've just pushed a commit that allows the Send button dropdown menu to be opened with either the Space or Enter key. Thanks for catching that! 👍

@JasonWeill
Copy link
Collaborator

When I use the keyboard to enter a prompt, TAB over to the dropdown arrow, press ENTER to open the menu, and then press ⬇️ and ENTER to send the message with the selection, it looks like the message is sent twice, once with the selection attached and once without.

image

@dlqqq
Copy link
Member Author

dlqqq commented Jul 29, 2024

@JasonWeill Good catch, thanks! Fixed in the latest revision.

Screen.Recording.2024-07-29.at.2.29.41.PM.mov

@JasonWeill
Copy link
Collaborator

Now, if I type a prompt foo, tab over to the dropdown, and press ENTER once, the menu appears, with nothing selected. When I press ENTER a second time, the prompt is sent without the selection, but the menu continues to appear. It seems like the sole menu item should be selected by default, or sending a message should close the menu if it's open.

image

@dlqqq
Copy link
Member Author

dlqqq commented Jul 29, 2024

@JasonWeill OK, I've re-enabled auto focusing for the first item. I had it disabled because I thought it looked ugly, but I'm willing to re-enable it for keyboard accessibility.

dlqqq added 9 commits July 29, 2024 18:26
This allows chat handlers to distinguish between the message prompt,
selection, and body (prompt + selection).

- The backend builds the message body from each `ChatRequest`, which
  consists of a `prompt` and a `selection`.

- The body will be used by most chat handlers, and will be used to
  render the messages in the UI.

- `/fix` uses `prompt` and `selection` separately as it does additional
  processing on each component. `/fix` does not use `body`.
@dlqqq dlqqq force-pushed the rework-selection-inclusion branch from 6d6f67a to b678b66 Compare July 29, 2024 22:26
@dlqqq dlqqq merged commit 79d66da into jupyterlab:main Jul 29, 2024
8 checks passed
@dlqqq dlqqq deleted the rework-selection-inclusion branch July 29, 2024 22:38
Marchlak pushed a commit to Marchlak/jupyter-ai that referenced this pull request Oct 28, 2024
* remove include selection checkbox

* add new selection types to backend

* add new TooltippedButton component

* add `prompt` field to `HumanChatMessage`

This allows chat handlers to distinguish between the message prompt,
selection, and body (prompt + selection).

- The backend builds the message body from each `ChatRequest`, which
  consists of a `prompt` and a `selection`.

- The body will be used by most chat handlers, and will be used to
  render the messages in the UI.

- `/fix` uses `prompt` and `selection` separately as it does additional
  processing on each component. `/fix` does not use `body`.

* add new include selection icon

* implement new send button

* pre-commit

* fix unit tests

* pre-commit

* edit cell selection tooltip to indicate output is not included

* allow Enter to open dropdown menu

* fix double-send bug when using keyboard

* re-enable auto focus for first item
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove "Include selection" checkbox
3 participants