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

Inline autocomplete tooltip UX changes #5149

Merged
merged 1 commit into from
May 10, 2023
Merged

Conversation

azmkercso
Copy link
Contributor

@azmkercso azmkercso commented May 2, 2023

Issue #, if available: N/A

Description of changes:
Changed UX of the inline tooltip for inline autocomplete.
Changes:

  • Icon buttons (for previous and next completion)
  • Displaying current completion's index and total completion index
  • Nicer UX for key binding display
  • Support dark mode
  • Display tooltip on active line hover or always (with toggle and delayed display/hide)
  • Separated tooltip and command code

It can be tried out in the kitchen sink demo, by using the Alt-C|Option-C keybinding to trigger its appearance together with the inline autocompletion. By default, the kitchen sink demo initialises the tooltip in hover mode, so it will be only displayed if the mouse is over the active line. This setting can be changed in the toolbar's overflow menu.

Updated revision:

  • Handles editor session change
  • Only render tooltip when the cursor is in the editor view and the tooltip can fit above the line properly
  • Improved tooltip repositioning
  • Keep editor focused after clicking on the tooltip

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

Patch coverage: 98.24% and project coverage change: +0.23 🎉

Comparison is base (475c6c3) 86.68% compared to head (1e86157) 86.92%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5149      +/-   ##
==========================================
+ Coverage   86.68%   86.92%   +0.23%     
==========================================
  Files         560      561       +1     
  Lines       44223    44900     +677     
  Branches     6853     6907      +54     
==========================================
+ Hits        38336    39028     +692     
+ Misses       5887     5872      -15     
Flag Coverage Δ
unittests 86.92% <98.24%> (+0.23%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/autocomplete.js 79.36% <75.00%> (+0.34%) ⬆️
src/ext/command_bar.js 96.30% <96.30%> (ø)
src/ext/command_bar_test.js 98.95% <98.95%> (ø)
src/ext/inline_autocomplete.js 91.81% <100.00%> (+3.02%) ⬆️
src/ext/inline_autocomplete_test.js 99.71% <100.00%> (+0.18%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@azmkercso azmkercso marked this pull request as ready for review May 2, 2023 20:41
@azmkercso azmkercso changed the title Inline autocomplete tooltip UX redesign Inline autocomplete tooltip UX changes May 3, 2023
@skazantsev
Copy link
Contributor

Minor bugs:

  • When the tooltip is shown on the last line it doesn’t disappear if you leave the tooltip by moving the cursor down.
  • When the tooltip is shown on the line 1-2 it may be shown off the screen. We might want to show the tooltip below the line.

@azmkercso azmkercso force-pushed the master branch 4 times, most recently from 5b2e65b to 074b5f5 Compare May 8, 2023 18:43
@azmkercso
Copy link
Contributor Author

@skazantsev The things you mentioned are fixed in the last revision.

Copy link
Contributor

@akoreman akoreman left a comment

Choose a reason for hiding this comment

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

lgtm, one concern I have with the inline autocomplete is that the suggestions are not communicated to screen reader users afaict, but that's outside the scope of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants