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.

Sign up for GitHub

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

Cosmetic changes #143

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Cosmetic changes #143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6ec692f
Add missing meta.brace.round scope
svipas Jun 14, 2019
ed0e351
Rename nord.json to nord-color-theme.json
svipas Jun 20, 2019
7f17611
Remove editorActiveLineNumber.foreground
svipas Jun 20, 2019
c5c6049
Replace editorWidgetBorder
svipas Jun 20, 2019
cb28e21
Replace editorGroup.background
svipas Jun 20, 2019
8661885
Remove legacy support of notification
svipas Jun 20, 2019
2fe22e8
Revert: Add missing meta.brace.round scope
svipas Jun 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
},
"license": "MIT",
"engines": {
"vscode": "^1.12.0"
"vscode": "^1.21.0"
Copy link
Contributor

@arcticicestudio arcticicestudio Jul 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

There is no advantage and necessary reason to increase the minimum version. This would exclude users that haven't updated yet or can not update for any reason, e.g. pre-configured workstation/systems.
Please also revert this change.

},
"galleryBanner": {
"color": "#2E3440",
Expand Down Expand Up @@ -59,7 +59,7 @@
{
"label": "Nord",
"uiTheme": "vs-dark",
"path": "./themes/nord.json"
"path": "./themes/nord-color-theme.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks a lot for pointing out this undocumented developer feature 💯
I've extracted this change into #148 to ensure this PR doesn't introduce multiple contextual changes.
Please revert this change or rebase on the latest develop branch which already includes this change.

}
]
},
Expand Down
20 changes: 2 additions & 18 deletions themes/nord.json → themes/nord-color-theme.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
"dropdown.background": "#3b4252",
"dropdown.border": "#3b4252",
"dropdown.foreground": "#d8dee9",
"editorActiveLineNumber.foreground": "#d8dee9cc",
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though this key has been deprecated in VS Code 1.22.0, again there is no reason to remove this key since it is deprecated but not invalid or unsupported by VS Code yet. The mechanism of deprecation is there for a reason: Allow legacy users to update and adapt to changes that will be unsupported in later versions.

What can be done here is to ensure it uses the same color like the new key (``) To ensure other contributors know about the deprecation we can add a comment:

Suggested change
"editorActiveLineNumber.foreground": "#d8dee9cc",
/*
* This key is deprecated as of VS Code version 1.22.0 in favour of `editorLineNumber.activeForeground`.
* See https://github.com/microsoft/vscode/commit/cc81646edfee5cd53fc082d73b43cc1c34db612c for details.
*/
"editorActiveLineNumber.foreground": "#d8dee9",

"editorCursor.foreground": "#d8dee9",
"editorHint.border": "#ebcb8b00",
"editorHint.foreground": "#ebcb8b",
Expand All @@ -32,7 +31,7 @@
"editorLineNumber.activeForeground": "#d8dee9",
"editorWhitespace.foreground": "#4c566ab3",
"editorWidget.background": "#2e3440",
"editorWidgetBorder": "#3b4252",
"editorWidget.border": "#3b4252",
Copy link
Contributor

Choose a reason for hiding this comment

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

This has already been fixed in #141
Please remove or rebase on the latest develop branch which already includes this change to also reduce the contextual changes in this PR.

"editor.background": "#2e3440",
"editor.foreground": "#d8dee9",
"editor.hoverHighlightBackground": "#3b4252",
Expand All @@ -54,7 +53,7 @@
"editorBracketMatch.background": "#2e344000",
"editorBracketMatch.border": "#88c0d0",
"editorCodeLens.foreground": "#4c566a",
"editorGroup.background": "#2e3440",
Copy link
Contributor

Choose a reason for hiding this comment

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

The addition of the new editorGroup.emptyBackground looks good to me, but again the deprecated key should stay.
Even though it was removed in microsoft/vscode@a970588 (VS Code 1.25.0: „Deprecated Theme colors“), they made sure to keep backwards compatibility microsoft/vscode#50773.
Please add the deprecated key again including a comment to ensure we know why this key is included:

Suggested change
"editorGroup.background": "#2e3440",
/*
* This key is deprecated as of VS Code version 1.25.0 in favour of `editorGroup.emptyBackground`.
* See https://github.com/microsoft/vscode/commit/a970588eb6ecb6772cd3e09c51a62d72914fdc7d,
* https://code.visualstudio.com/updates/v1_25#_deprecated-theme-colors and
* https://github.com/microsoft/vscode/issues/50773 for more details.
*/
"editorGroup.background": "#2e3440",

"editorGroup.emptyBackground": "#2e3440",
"editorGroup.border": "#3b425201",
"editorGroup.dropBackground": "#3b425299",
"editorGroupHeader.noTabsBackground": "#2e3440",
Expand Down Expand Up @@ -129,20 +128,6 @@
"merge.incomingHeaderBackground": "#8fbcbb66",
"merge.incomingContentBackground": "#8fbcbb4d",
"merge.border": "#3b425200",

/* `notification.*` keys are legacy support for VS Code versions >1.21.0 */
Copy link
Contributor

Choose a reason for hiding this comment

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

Again like the other deprecated keys removed in this PR: For now Nord supports older versions at least until these keys are removed from VS Code itself or a major update is released making these keys unsupported.
Please revert the change back too.

"notification.background": "#3b4252",
"notification.buttonBackground": "#434c5e",
"notification.buttonForeground": "#d8dee9",
"notification.buttonHoverBackground": "#4c566a",
"notification.errorBackground": "#bf616a",
"notification.errorForeground": "#2e3440",
"notification.foreground": "#d8dee9",
"notification.infoBackground": "#88c0d0",
"notification.infoForeground": "#2e3440",
"notification.warningBackground": "#ebcb8b",
"notification.warningForeground": "#2e3440",

"notificationCenter.border": "#3b425200",
"notificationCenterHeader.background": "#2e3440",
"notificationCenterHeader.foreground": "#88c0d0",
Expand All @@ -151,7 +136,6 @@
"notifications.border": "#2e3440",
"notifications.foreground": "#d8dee9",
"notificationToast.border": "#3b425200",

Copy link
Contributor

Choose a reason for hiding this comment

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

This separation blank line is intended and should stay.

"panel.background": "#2e3440",
"panel.border": "#3b4252",
"panelTitle.activeBorder": "#88c0d000",
Expand Down