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

feat: add keyboard shortcut (ctrl/cmd-k) to clear console #1680

Merged
merged 3 commits into from
Mar 6, 2025

Conversation

knqu
Copy link
Contributor

@knqu knqu commented Mar 6, 2025

Fixes #1557

Add CTRL/CMD+k keyboard shortcut to clear the console.

@knqu knqu requested review from codebytere and a team as code owners March 6, 2025 02:08
@knqu
Copy link
Contributor Author

knqu commented Mar 6, 2025

Having some trouble figuring out what's going wrong with these tests. This is my first time trying to contribute to an open source project, so I'd greatly appreciate any guidance from those more experienced with this project. The changes I made appear to work when I try to run Fiddle (ctrl+k) does indeed clear the console, but I'm failing a CI test with the following error: TypeError: Cannot read properties of undefined (reading 'CtrlCmd'). I spent considerable time trying to debug this, but couldn't find any inconsistencies between my code and the documentation for the Monaco Editor. Any advice would be much appreciated!

@BlackHole1
Copy link
Member

Hi @knqu. The error in the unit tests is because we are using a Mock monaco solution in the tests, so when you use the Monaco API in the existing code, you need to add the same API in the Mock data.

See:

diff --git a/tests/mocks/monaco.ts b/tests/mocks/monaco.ts
index 0a5c9404..1cca8287 100644
--- a/tests/mocks/monaco.ts
+++ b/tests/mocks/monaco.ts
@@ -44,6 +44,12 @@ export class MonacoMock {
       },
     },
   };
+  public KeyMod = {
+    CtrlCmd: jest.fn(),
+  };
+  public KeyCode = {
+    KEY_K: jest.fn(),
+  };
 }
 
 export class MonacoEditorMock {
@@ -55,6 +61,7 @@ export class MonacoEditorMock {
   private model = new MonacoModelMock('', 'javascript');
   private scrollHeight = 0;
 
+  public addCommand = jest.fn();
   public dispose = jest.fn();
   public getAction = jest.fn(() => this.action);
   public getModel = jest.fn(() => this.model);

@knqu
Copy link
Contributor Author

knqu commented Mar 6, 2025

Thanks for the pointer @BlackHole1! I'll definitely keep that in mind going forward.

@codebytere
Copy link
Member

@knqu could you please also add some logic to reset the console? From an abandoned PR:

https://github.com/electron/fiddle/pull/1669/files#diff-51ceea48515cf88b9698ed88bb3b808d1bdfe268fca34ba71bdf740e0f6cbb81R100-R106

once that's done we can merge this.

@knqu
Copy link
Contributor Author

knqu commented Mar 6, 2025

Hi @codebytere, thanks for getting back to me! That's a smart idea; I added it to my code. From my testing, the scroll logic seemed redundant, as clearing the console means there's nothing to scroll above, so I omitted that line. Please let me know if you have any other feedback!

@codebytere codebytere merged commit dbdfa73 into electron:main Mar 6, 2025
8 checks passed
@coveralls
Copy link

coveralls commented Mar 6, 2025

Coverage Status

coverage: 87.472% (-0.03%) from 87.502%
when pulling ba4db50 on knqu:clear-console
into 39848ed on electron:main.

@knqu knqu deleted the clear-console branch March 6, 2025 17:22
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.

cmd+k to clear the console
4 participants