-
Notifications
You must be signed in to change notification settings - Fork 77
Add space menu and window menu #402
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
Conversation
4e3f332 to
314bfbf
Compare
71
left a comment
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.
Thank you! A couple minor comments.
extensions/helix/package.build.ts
Outdated
| ); | ||
| `.split("\n").map(line => line.trim()).join(""), | ||
| ], |
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.
nit: you can also keep this as an array to be more readable (also removing the opening [)
| ], | |
| `.trim().split("\n"), |
To remove the indentation, I would do .replaceAll(/^ {,18}/mg, "") or something like this to preserve the "internal" indentation.
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.
Sorry if that wasn't clear, I meant writing the text multiline using a template literal
code: `
if (...) {
...
}`.replaceAll(/^ {,2}/mg, "")This way it is a multiline string which is easy to read and edit in package.build.ts, and in the generated package.json it is an array of lines which is similarly easy to read (but less so than a multiline string).
extensions/helix/package.build.ts
Outdated
| const quickOpenPrefix = relativeDirectoryPath.endsWith('/') | ||
| ? relativeDirectoryPath | ||
| : \`\${relativeDirectoryPath}/\`; | ||
| await vscode.commands.executeCommand( | ||
| 'workbench.action.quickOpen', | ||
| quickOpenPrefix | ||
| ); |
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.
Oh that's a neat idea!
extensions/helix/package.build.ts
Outdated
| // "n": { Not easily possible. Neccessary? | ||
| // text: "New split scratch buffer", | ||
| // command: "", | ||
| // }, |
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.
No need to do it here, but perhaps we can check if the editor has horizontal / vertical splits already (maybe with vscode.window.tabGroups?), create them if needed (maybe vscode.window.showTextDocument() or using View: Split Editor), and then an empty file there? Also I'm not sure all of these steps are possible.
71
left a comment
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.
A few nits, but also the confusing indentation made us miss a problem: the window menu is defined within the space object, so we can't actually open it with w.
extensions/helix/package.build.ts
Outdated
| ); | ||
| `.split("\n").map(line => line.trim()).join(""), | ||
| ], |
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.
Sorry if that wasn't clear, I meant writing the text multiline using a template literal
code: `
if (...) {
...
}`.replaceAll(/^ {,2}/mg, "")This way it is a multiline string which is easy to read and edit in package.build.ts, and in the generated package.json it is an array of lines which is similarly easy to read (but less so than a multiline string).
|
I don't understand why CI failed, but it should have followed the indentation correctly. |
I think it wants the "following lines" to be indented wrt the property being defined. This fixed it for me: diff --git a/extensions/helix/package.build.ts b/extensions/helix/package.build.ts
index 221ba72..d29a928 100644
--- a/extensions/helix/package.build.ts
+++ b/extensions/helix/package.build.ts
@@ -307,10 +307,10 @@ export const pkg = (modules: Builder.ParsedModule[]) => ({
'workbench.action.quickOpen',
quickOpenPrefix,
);`
- // The indentation of multi-line template strings
- .replaceAll(" ", "")
- .replaceAll(/^ {,2}/mg, "")
- .split("\n")
+ // The indentation of multi-line template strings
+ .replaceAll(" ", "")
+ .replaceAll(/^ {,2}/mg, "")
+ .split("\n")
,
},
],As a small nit (since we need to change this code anyway), I think the following is a bit more readable: diff --git a/extensions/helix/package.build.ts b/extensions/helix/package.build.ts
index 221ba72..7588f4a 100644
--- a/extensions/helix/package.build.ts
+++ b/extensions/helix/package.build.ts
@@ -307,10 +307,9 @@ export const pkg = (modules: Builder.ParsedModule[]) => ({
'workbench.action.quickOpen',
quickOpenPrefix,
);`
- // The indentation of multi-line template strings
- .replaceAll(" ", "")
- .replaceAll(/^ {,2}/mg, "")
- .split("\n")
+ // The indentation of multi-line template strings
+ .replaceAll(" ".repeat(18), "")
+ .split("\n")
,
},
],There is another edge case with diff --git a/extensions/helix/package.build.ts b/extensions/helix/package.build.ts
index 221ba72..3f4ddfa 100644
--- a/extensions/helix/package.build.ts
+++ b/extensions/helix/package.build.ts
@@ -295,7 +295,7 @@ export const pkg = (modules: Builder.ParsedModule[]) => ({
const currentFileUri = editor.document.uri;
const currentDirectoryUri = vscode.Uri.joinPath(currentFileUri, '..');
const workspaceFolder = vscode.workspace.getWorkspaceFolder(currentFileUri);
- if (!workspaceFolder) {
+ if (!workspaceFolder || currentDirectoryUri.fsPath === workspaceFolder.uri.fsPath) {
return await fallback();
}
const relativeDirectoryPath = vscode.workspace.asRelativePath( |
|
Thanks for your suggestion, but I've found a better way to get the indentation. |
71
left a comment
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.
Thank you!
This keybinding seems to have been forgotten in #402 Co-authored-by: jgdhs27 <jgdhs27@gmail.com>
Not implemented:
j: Open jumplist picker': Open last pickerd: Open diagnostic pickerVS Code doesn’t have a single-file diagnostic picker, so it currently behaves the same as
D.The window menu was copied directly from Silverquark’s fork.