-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
Possibility to set statusBar foreground color #2350
Conversation
Travis tests have failedHey Mikael Göransson, Node.js: 8.9.1git ls-tree -r HEAD --name-only | grep ".*.[t|j]s$" | xargs ./node_modules/prettier/bin/prettier.js --write --print-width 100 --single-quote --trailing-comma es5
gulp
|
@@ -31,15 +31,22 @@ class StatusBarClass implements vscode.Disposable { | |||
} | |||
} | |||
|
|||
public SetColor(color: string) { | |||
public SetColor(background: string, foreground?: string) { | |||
const currentColorCustomizations = vscode.workspace |
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.
Can you call Configuration.getConfiguration
here?
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.
You mean instead of calling vscode.workspace.getConfiguration('workbench').get('colorCustomizations')
I should use something like Configuration.getConfiguration('workspace').get('colorCustomizations')
? (not able to look up the exact syntax right now :)). If so, then it should be used when updating the configuration as well?
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.
Actually scratch that, I realize this fx is not exposed in IConfiguration
.
src/statusBar.ts
Outdated
}; | ||
if (foreground !== undefined) { | ||
newColorCustomizations['statusBar.foreground'] = `${foreground}`; | ||
} else if (currentColorCustomizations !== undefined && 'statusBar.foreground' in currentColorCustomizations) { |
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.
Should be sufficient to only have an else
, and run delete currentColorCustomizations['statusBar.foreground'];
if foreground is undefined.
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.
That would result in [ts] Object is possibly 'undefined'
.
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.
Actually, I realized that the delete
will modify the object, but won't actually update it with VSCode.
This should be better:
public SetColor(background: string, foreground?: string) {
const currentColorCustomizations = vscode.workspace
.getConfiguration('workbench')
.get('colorCustomizations');
let colorCustomizations = Object.assign(currentColorCustomizations, {
'statusBar.background': `${background}`,
'statusBar.noFolderBackground': `${background}`,
'statusBar.debuggingBackground': `${background}`,
});
if (foreground === undefined) {
delete colorCustomizations['statusBar.foreground'];
} else {
colorCustomizations['statusBar.foreground'] = `${foreground}`;
}
vscode.workspace
.getConfiguration('workbench')
.update('colorCustomizations', colorCustomizations, true);
}
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 taking this on. Can you address CI failures?
Sorry for late response, I have had a hectic week. I'll fix it during the weekend, when my additional questions has been cleared out. |
479bdb6
to
5c8b17c
Compare
Basically a reimplementation of pull#2102. Updated documentation to include instructions on how colors are set.
5c8b17c
to
aa8deb3
Compare
Sorry for taking so long to merge this in. Still on vacation. I'll merge as soon as this new build passes CI. |
What this PR does / why we need it:
Basically a reimplementation of #2102. Updated documentation to include instructions on how colors are set.
Having the possibility to set the foreground color makes it easier to select a background color and get a good contrast between the two.
Which issue(s) this PR fixes
N/A.
Special notes for your reviewer:
N/A.