[Console] Monaco migration: send request and copy as curl buttons #179808
Conversation
yuliacech
force-pushed
the
console/implement_editor_actions
branch
2 times, most recently
from
7dd8c03
to
f1384bd
Compare
yuliacech
force-pushed
the
console/implement_editor_actions
branch
from
f1384bd
to
bb3d4fb
Compare
yuliacech
added
Feature:Console
| import { CONSOLE_LANG_ID } from './constants'; | ||
| import { monaco } from '../monaco_imports'; | ||
|
|
||
| export const setupConsoleErrorsProvider = (workerProxyService: ConsoleWorkerProxyService) => { |
There was a problem hiding this comment.
this file is copied from the previously used file packages/kbn-monaco/src/ace_migration/setup_worker.ts
| import { CONSOLE_LANG_ID } from './constants'; | ||
| import { ConsoleParserResult, ConsoleWorkerDefinition } from './types'; | ||
|
|
||
| export class ConsoleWorkerProxyService { |
There was a problem hiding this comment.
this file is copied from the previously used file packages/kbn-monaco/src/ace_migration/worker_proxy.ts
|
/ci |
|
/ci |
yuliacech
changed the title
|
Pinging @elastic/kibana-management (Team:Kibana Management) |
There was a problem hiding this comment.
Thanks for the ping, @yuliacech! I'm coming in a bit sideways, so I've left you a handful of comments/questions for my initial review. Let me know if there's anything than needs additional clarity.
| <EuiLink | ||
| color="success" | ||
| onClick={sendRequestsCallback} | ||
| data-test-subj="sendRequestButton" | ||
| aria-label={i18n.translate('console.sendRequestButtonTooltip', { | ||
| defaultMessage: 'Click to send request', | ||
| })} | ||
| > | ||
| <EuiIcon type="playFilled" /> |
There was a problem hiding this comment.
Would it be better to use the EuiButtonIcon component here, instead of EuiLink?
| <ConsoleMenu | ||
| getCurl={getCurlCallback} | ||
| getDocumentation={() => { | ||
| return Promise.resolve(null); | ||
| }} | ||
| autoIndent={() => {}} | ||
| notifications={notifications} | ||
| /> |
There was a problem hiding this comment.
Would it be better to use the EuiButtonIcon component here, instead of EuiLink?
src/plugins/console/public/application/containers/editor/monaco/monaco_editor.tsx
Outdated
Show resolved
Hide resolved
src/plugins/console/public/application/containers/editor/monaco/monaco_editor.tsx
Outdated
Show resolved
Hide resolved
src/plugins/console/public/application/containers/editor/monaco/monaco_editor.tsx
Show resolved
Hide resolved
src/plugins/console/public/application/containers/editor/monaco/monaco_editor.tsx
Show resolved
Hide resolved
src/plugins/console/public/application/containers/editor/monaco/monaco_editor.tsx
Show resolved
Hide resolved
src/plugins/console/public/application/containers/editor/monaco/monaco_editor.tsx
Show resolved
Hide resolved
| <ConsoleMenu | ||
| getCurl={getCurlCallback} | ||
| getDocumentation={() => { | ||
| return Promise.resolve(null); | ||
| }} | ||
| autoIndent={() => {}} | ||
| notifications={notifications} | ||
| /> |
There was a problem hiding this comment.
The current "Open documentation" button doesn't appear to open any Elastic documentation that I can see. Is it supposed to navigate me to or open a new browser tab to the full Elastic docs?
| <ConsoleMenu | ||
| getCurl={getCurlCallback} | ||
| getDocumentation={() => { | ||
| return Promise.resolve(null); | ||
| }} | ||
| autoIndent={() => {}} | ||
| notifications={notifications} | ||
| /> |
There was a problem hiding this comment.
Is the current "Open documentation" action intend to be contextual to the currently highlighted command? If not, would both "Open documentation" and "Auto indent" actions be better served as global actions rather than individual command actions?
There was a problem hiding this comment.
Amazing job @yuliacech! I tested locally and everything implemented so far works well (apart from the highlighting issue but this is not related to this PR). I left a few comments where I thought documentation or unit tests could be added.
| import { CONSOLE_LANG_ID } from './constants'; | ||
| import { monaco } from '../monaco_imports'; | ||
|
|
||
| export const setupConsoleErrorsProvider = (workerProxyService: ConsoleWorkerProxyService) => { |
There was a problem hiding this comment.
Might be helpful to add some documentation here that explains what this function does.
| import { ParsedRequest } from './types'; | ||
| import { monaco } from '../monaco_imports'; | ||
|
|
||
| export class ConsoleParsedRequestsProvider { |
There was a problem hiding this comment.
Documentation would be helpful here as well.
| import { CONSOLE_LANG_ID } from './constants'; | ||
| import { ConsoleParserResult, ConsoleWorkerDefinition } from './types'; | ||
|
|
||
| export class ConsoleWorkerProxyService { |
There was a problem hiding this comment.
Documentation would be helpful here as well.
| const { requests } = parser(input) as ConsoleParserResult; | ||
| expect(requests.length).toBe(2); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
I wonder if we could also add test cases for requests with data to make sure it is parsed correctly. Wdyt?
| import type { DevToolsVariable } from '../../../components'; | ||
| import { EditorRequest } from './monaco_editor_actions_provider'; | ||
| import { MetricsTracker } from '../../../../types'; | ||
|
|
There was a problem hiding this comment.
We could add some unit tests for these util functions.
|
Thanks a lot for your review, @MichaelMarcialis! I implemented your suggestions for the buttons, except for usage of |
|
Thanks a lot for your review, @ElenaStoeva! |
There was a problem hiding this comment.
Thanks for making those changes, @yuliacech! Looks great.
I implemented your suggestions for the buttons, except for usage of EuiButtonIcon: it renders the icons with some padding that messes up the alignment of the buttons on the line.
So I kept EuiLink with a nested icon for now, since it results in the same html structure as EuiButtonIcon anyways (button with an svg). Let me know if you think we should address this in a follow up issue.
Yeah, this might be worthwhile to revisit in a follow-up issue. While the resulting HTML may be the same or similar, the button component styling is a bit more robust in regard to the hover/focus/active states, while the link component for icon-only content is a bit lacking. Things that jump to mind that may remedy the alignment issue you reference could be to 1) increase the console editor's line height, 2) override the EUI button styles to be slightly smaller, or 3) some combination of the two.
To answer your question about documentation and auto-indent buttons: this functionality is not yet implemented, we have #180209 and #180213 to to track this work. Both actions apply to the "current" request, so the browser will open a new tab with the docs for the specific request and also indentations are applied only to the selected request.
Gotcha. In that case then, the only concern I'd mention currently is that the documentation link doesn't work in some situations. For example, incomplete or invalid commands appear to result in nothing happening when attempting to view the documentation. Should we disable or hide the documentation link in those conditional situations?
Otherwise, this looks good from my perspective. Approving now so I don't hold you up further.
|
Opened #180911 to follow up on the action buttons |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
History
To update your PR or re-run it, just comment with: |
Summary
Closes #178990
This PR adds the actions buttons to the monaco editor and the functionality for the buttons to send a request and copy the request as a curl command. When the cursor or user selection doesn't overlap or contain any requests on the selected line, the actions buttons are hidden. When the cursor or selection changes, the buttons are displayed on the 1st line of the 1st selected request. Several requests can be sent at once. Only the 1st request is copied as a curl command.
There are also some minor UI and copy changes (see screenshots below) as suggested by @MichaelMarcialis in the review.
Screenshot
Before
After
How to test
Follow up issues