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

[Console Migration] Add action for auto indentation #181613

Merged

Conversation

ElenaStoeva
Copy link
Contributor

@ElenaStoeva ElenaStoeva commented Apr 24, 2024

Closes #180213

Summary

This PR adds an action for applying indentation to the selected requests in the Console Monaco editor.

Screen.Recording.2024-05-14.at.16.20.03.mov

Note: This PR doesn't auto-indent requests that contain comments. This will be part of a follow-up work (#182138).

@ElenaStoeva ElenaStoeva added Feature:Console Dev Tools Console Feature Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes labels Apr 24, 2024
@ElenaStoeva ElenaStoeva self-assigned this Apr 24, 2024
@ElenaStoeva ElenaStoeva marked this pull request as ready for review April 30, 2024 13:48
@ElenaStoeva ElenaStoeva requested a review from a team as a code owner April 30, 2024 13:48
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-management (Team:Kibana Management)

Copy link
Contributor

@yuliacech yuliacech left a comment

Choose a reason for hiding this comment

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

Thanks a lot for implementing the auto-indent, @ElenaStoeva!
Since loosing any user content might be an issue, let's discuss in the team sync on how to approach auto-indentation of comments

@ElenaStoeva ElenaStoeva marked this pull request as draft May 8, 2024 11:51
@ElenaStoeva ElenaStoeva force-pushed the console-migration/auto-indentation branch from c2ebb1d to 09f5333 Compare May 8, 2024 16:02
@ElenaStoeva
Copy link
Contributor Author

/ci

@ElenaStoeva ElenaStoeva marked this pull request as ready for review May 9, 2024 18:29
@ElenaStoeva ElenaStoeva requested a review from a team as a code owner May 9, 2024 18:29
@ElenaStoeva ElenaStoeva requested a review from yuliacech May 9, 2024 18:35

const autoIndentedText = getAutoIndentedRequests(parsedRequests, selectedText);

this.editor.executeEdits('', [
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think for completeness sake we should use some kind of string to indicate that the edit was made by the auto-indent button

Copy link
Contributor Author

@ElenaStoeva ElenaStoeva May 14, 2024

Choose a reason for hiding this comment

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

Ah I wasn't sure how this string would be used... will it be visible to the user somehow or do you think we'll only need it for internal use? I added a translated string (in case it will be displayed to the user), let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the string might be used for undo/redo actions which I don't think we currently have. I believe the value won't be displayed in the UI to the user though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see... Then it might be good to store this in a constant in case it's needed somewhere. I added this change with 3e1264f.

Copy link
Contributor

@yuliacech yuliacech left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this feature, @ElenaStoeva!
I think that is a good approach to only auto-indent the requests that don't contain any comments.
I think though that we don't need to keep track of the text of the request in the parser: You are already using the model to get the text in the range of selected requests with model.getValueInRange. Each request also has startLineNumber and endLineNumber. If you pass the model to the util auto-indent function, you should be able to iterate over the requests array and for each you should know when it's the start line with the method (startLineNumber) and where the request body begins (startLineNumber + 1) and where the request body ends (endLineNumber). With this line numbers you can get the text from the model and do the same check for comments. WDYT?

@ElenaStoeva ElenaStoeva force-pushed the console-migration/auto-indentation branch from 4f3da2b to 50363cc Compare May 14, 2024 15:24
@ElenaStoeva
Copy link
Contributor Author

Thanks a lot for working on this feature, @ElenaStoeva! I think that is a good approach to only auto-indent the requests that don't contain any comments. I think though that we don't need to keep track of the text of the request in the parser: You are already using the model to get the text in the range of selected requests with model.getValueInRange. Each request also has startLineNumber and endLineNumber. If you pass the model to the util auto-indent function, you should be able to iterate over the requests array and for each you should know when it's the start line with the method (startLineNumber) and where the request body begins (startLineNumber + 1) and where the request body ends (endLineNumber). With this line numbers you can get the text from the model and do the same check for comments. WDYT?

Thanks for the review @yuliacech! You're right, we don't need to add a text property to the parsed requests. I refactored the auto-indent util function so that we don't need this property from the parser. Instead of passing the editor model to the util function as you suggested, I decided to take a slightly different approach, and obtain all needed text from the editor inside the actions provider function. This way, we can isolate the editor model from the util functions to minimize complexity and allow easier unit testing of the util function. Let me know what you think.

@ElenaStoeva ElenaStoeva force-pushed the console-migration/auto-indentation branch from 845ccf7 to addacb6 Compare May 14, 2024 15:45
@@ -74,6 +74,10 @@ export const MonacoEditor = ({ initialTextValue }: EditorProps) => {
return actionsProvider.current!.getDocumentationLink(docLinkVersion);
}, [docLinkVersion]);

const autoIndentCallback = useCallback(async (event: React.MouseEvent) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the argument "event" is now unused and can be deleted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! I removed it in 3e1264f.

Copy link
Contributor

@yuliacech yuliacech left a comment

Choose a reason for hiding this comment

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

Thanks a lot for addressing my comments, @ElenaStoeva!
Latest changes LGTM 👍

@ElenaStoeva ElenaStoeva enabled auto-merge (squash) May 15, 2024 12:44
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
console 447.8KB 448.9KB +1.1KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @ElenaStoeva

@ElenaStoeva ElenaStoeva merged commit 19818f2 into elastic:main May 15, 2024
20 checks passed
@kibanamachine kibanamachine added v8.15.0 backport:skip This commit does not require backporting labels May 15, 2024
@ElenaStoeva ElenaStoeva deleted the console-migration/auto-indentation branch June 24, 2024 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Console Dev Tools Console Feature release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Console Monaco migration] Action button to auto-indent
6 participants