-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 horizontal resizability to autocomplete popup #180243
Add horizontal resizability to autocomplete popup #180243
Conversation
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 for working on this @sakurai-youhei. I tested locally and the resizability works well overall. However, I noticed a small visual regression - now the side bar covers the last letter of each word specifying the type of the suggestion:
You can uncover them by extending the autocomplete popup to the right, but they are covered in the initial state when the popup appears and I don't think this looks good visually. Would there be any fix for this?
Also, please keep in mind that we are in the process of migrating Console to Monaco editor (see #180207) so we might need to implement this there as well once we complete the migration.
@ElenaStoeva Thank you for finding the regression. I didn't notice it because it didn't impact Chrome on my laptop. I've fixed it in the latest commit. Regarding the new Console, Resize in new Console
/*
* https://github.com/elastic/kibana/blob/main/packages/shared-ux/code_editor/impl/code_editor.tsx#L65-L70
* https://microsoft.github.io/monaco-editor/docs.html#interfaces/languages.CompletionItemProvider.html
* https://microsoft.github.io/monaco-editor/playground.html?source=v0.44.0#example-extending-language-services-completion-provider-example
*/
suggestionProvider={
new (class implements monaco.languages.CompletionItemProvider {
provideCompletionItems(model: monaco.editor.ITextModel, position: monaco.Position) {
const word = model.getWordUntilPosition(position);
const range = {
startLineNumber: position.lineNumber,
endLineNumber: position.lineNumber,
startColumn: word.startColumn,
endColumn: word.endColumn,
};
return {
suggestions: Array(20).fill({
label: '"test-label"',
kind: monaco.languages.CompletionItemKind.Function,
documentation: 'test documentation',
insertText: '"test": "*"',
range,
}),
};
}
})()
} |
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 for addressing my feedback @sakurai-youhei! Tested again and now it looks good to me.
Regarding the new Console, Monaco Editor has built-in horizontal and vertical resizers that will be activated by one or more suggestions even in the new Console. The rest is definitely off-topic, but the following is a quick implementation of the suggestionProvider and how it looks.
Noted, thanks! We'll keep this in mind when we implement the autocomplete functionality in the Console Monaco editor. cc: @yuliacech
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 @sakurai-youhei! I've added some comments below for my initial review.
…ndle in Safari Co-authored-by: Michael Marcialis <[email protected]>
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 for the change and comments @sakurai-youhei. I've responded to your comments below. Assuming you're able to address those responses, I'll go ahead and approve this now to unblock you for feature freeze.
@MichaelMarcialis I changed how to expose the resizer on Safari and updated the PR description accordingly. I believe the negative z-index doesn't create a new incompatibility because #151386 has already introduced it. But I know it doesn't mean zero concerns around this negative z-index trick. Although I've gone through browsers again, I appreciate your review & checks again. If you find this way is not preferable, please let me know, and I will try to find another. |
💚 Build Succeeded
Metrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: |
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.
LGTM. Thanks!
Summary
This PR adds horizontal resizability to the autocomplete popup. The resized width does not persist, so it returns to the default on every load.
^^^ Demonstrated on Chrome Version 124.0.6367.91 (Official Build) (64-bit).
Appearance on Edge Version Version 124.0.2478.51 (Official build) (64-bit)
Appearance on Firefox 125.0.2 (64-bit)
Appearance on Safari 17.2.1 (19617.1.17.11.12)
Show scroll bars=Automatically
Show scroll bars=Always
Appearance on iOS Safari 17.4.1 - popup does not looks good same as before
vvvThis branch:
vvv8.13.0:
Appearances at 232897c
^^^ Demonstrated on Chrome Version 123.0.6312.123 (Official Build) (64-bit).
Appearance on Edge Version 123.0.2420.81 (Official build) (64-bit)
Appearance on Firefox 124.0.2 (64-bit)
Appearance on Safari 17.2.1 (19617.1.17.11.12)
Appearances at 98864be
^^^ Demonstrated on Chrome Version 123.0.6312.106 (Official Build) (64-bit).
Appearance on Edge Version 123.0.2420.81 (Official build) (64-bit)
Appearance on Firefox 124.0.2 (64-bit)
Appearance on Safari 17.2.1 (19617.1.17.11.12)
Closes #171268
Closes #125186
Justification
The autocomplete popup width has been fixed to
280px
HERE for a long time. Extending it would also be appreciated as ace 1.32.9 defaults it to300px
, and other projects using the ace make it even wider. However, this PR doesn't scope the extension because it's not easy to determine the best width. Furthermore, automatic fitting to suggestions' width is also excluded from the scope to avoid generating new corner cases.A horizontal scrollbar is an alternative solution, but it's less cost-effective because it requires writing code to sync two separate DOM elements,
.ace_scrollbar.ace_scrollbar-h
and.ace_scroller
.For keyboard accessibility, it's definitely best to add two shortcuts, making the autocomplete popup wider and narrower. However, the questions are what key combinations are natural for them and what delta is appropriate per expansion/shrink. This could be addressed as another enhancement if key combinations were suggested at least.
CSS resizer on Safari
The CSS resizer behaves uniquely on Safari. You can't resize an element if another element overlaps the resizer like this.
Safari.17.2.1.2024-04-18.23.20.05.mov
The workaround is to set a negative z-index to the overlapping element like this.
Checklist
For maintainers
Release note
Adds horizontal resizability to the autocomplete popup on the Dev Tools Console