Cosmetic changes #143

svipas · 2019-06-20T16:14:51Z

Rename nord.json to nord-color-theme.json. It would show color palette, warnings/errors, etc. when the file is opened.
Remove editorActiveLineNumber.foreground it's deprecated in favor of editorLineNumber.activeForeground.
Replace deprecated editorWidgetBorder to editorWidget.background.
Replace deprecated editorGroup.background to editorGroup.emptyBackground.
Remove legacy support of notifications and increase min VS Code version to 1.21.0. I believe no one is using so old VS Code version when the current version is already 1.35.1 and soon it would be a 1.36.0.

- It would show color palette, warnings/errors, etc. when the file is opened.

- Also, increase min. version of VS Code.

arcticicestudio · 2019-07-20T15:47:20Z

Hi @svipas 👋, thanks for your contribution 👍
Thanks for all these changes, but in order to review PRs and keep the project in a maintainable state each improvement/bug fix/feature should be submitted as a topic-focused single PR without contextual mixes. If you don't have the time to split these changes I can extract the changes into single PRs for you.

Also I've searched the documentations, but haven't found any mentions regarding the name of the color theme file (nord.json ➜ nord-color-theme.json) to enable additional development features. It this a new and currently undocumented features?

Regarding the removed notification keys: Even when the version of VS Code is long outdated and maybe not used by a lot of users anymore (but it is still possible), the keys are not affecting the performance or appearance and I'd like to support them at least until VS Code removes them completely or in theme version 1.0.0. This ensures all users have at least the chance to update to a later version.

arcticicestudio · 2019-07-20T19:52:58Z

I've checked some bundled and default themes and it looks like the name of the theme file is indeed an undocumented feature when suffixed with -color-theme.json. It enables JSON schema validation for VS Code's color theme API as well as color previews (color picker) for HEX colors.
I guess we should adapt to the naming scheme, there will be no breaking change since the theme is identified by it's extension ID as well as the _metadata_ field in the package.json and not by the name of the theme file (which would be odd since a theme can provide multiple theme files).

@svipas

An undocumented feature for theme extension developers is to ensure the name of the JSON file is suffixed with `-color-theme.json`. This enables the JSON scheme validation for the theme API allowing developers to validate the implemented theme keys, showing warnings about deprecated keys and providing full auto completion, field documentations and color previews (color picker) for the HEX format. This change aligns Nord to the official bundled and default themes (1) by adapting to the naming scheme without being a breaking change since the theme is identified by it's extension ID as well as the `_metadata` field in the `package.json` and not by the name of the theme file (which would be odd since a theme can provide multiple theme files). References: (1) https://github.com/microsoft/vscode/tree/master/extensions/theme-abyss/themes (2) https://github.com/svipas Extracted from GH-143 submitted by @svipas (2) Resolves GH-148

arcticicestudio · 2019-07-21T07:32:18Z

package.json

@@ -19,7 +19,7 @@
  },
  "license": "MIT",
  "engines": {
-    "vscode": "^1.12.0"
+    "vscode": "^1.21.0"


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.

arcticicestudio · 2019-07-21T07:34:07Z

package.json

@@ -59,7 +59,7 @@
      {
        "label": "Nord",
        "uiTheme": "vs-dark",
-        "path": "./themes/nord.json"
+        "path": "./themes/nord-color-theme.json"


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.

arcticicestudio · 2019-07-21T07:41:31Z

themes/nord-color-theme.json

@@ -22,7 +22,6 @@
    "dropdown.background": "#3b4252",
    "dropdown.border": "#3b4252",
    "dropdown.foreground": "#d8dee9",
-    "editorActiveLineNumber.foreground": "#d8dee9cc",


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",

arcticicestudio · 2019-07-21T07:43:00Z

themes/nord-color-theme.json

@@ -32,7 +31,7 @@
    "editorLineNumber.activeForeground": "#d8dee9",
    "editorWhitespace.foreground": "#4c566ab3",
    "editorWidget.background": "#2e3440",
-    "editorWidgetBorder": "#3b4252",
+    "editorWidget.border": "#3b4252",


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.

arcticicestudio · 2019-07-21T13:28:47Z

themes/nord-color-theme.json

@@ -151,7 +136,6 @@
    "notifications.border": "#2e3440",
    "notifications.foreground": "#d8dee9",
    "notificationToast.border": "#3b425200",
-


This separation blank line is intended and should stay.

arcticicestudio · 2019-07-21T13:30:21Z

themes/nord-color-theme.json

@@ -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 */


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.

arcticicestudio · 2019-07-21T13:41:04Z

themes/nord-color-theme.json

@@ -54,7 +53,7 @@
    "editorBracketMatch.background": "#2e344000",
    "editorBracketMatch.border": "#88c0d0",
    "editorCodeLens.foreground": "#4c566a",
-    "editorGroup.background": "#2e3440",


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",

arcticicestudio

I've reviewed your PR and added some change requests. Please don't get me wrong, every contribution is always welcome 💪, but a large PR with many different contextual changes are hard to review and harder to debug later on when something needs to be reverted or when trying to search when and why a specific change was made.
I'd like to merge your PR as soon as all open PR discussion are resolved.

svipas · 2019-10-04T18:28:29Z

Hi @arcticicestudio do you want me to fix conflicts mentioned in comments? I'm asking to know if this PR is still relevant.

svengreb · 2023-08-09T08:17:31Z

Thank you for your patience! 🙏🏼
It‘s been a while since I had free time to focus more on Nord, and my open source projects in general, and invest time in this issue due to work-life balance.

I recently published the first “Northern Post — The state and roadmap of Nord“ announcement which includes all details about the plans and future of the Nord project, including the goal of catching up with the backlog. This issue is part of the backlog and therefore I want to triage and process it to get one step closer to a “clean state“. Read the announcement about reaching the “clean“ contribution triage state in Nord‘s discussions for more details about the goal.

Therefore it has been added to the queue in the central and single-source-of-truth project board that is also described in more detail in the roadmap announcement.

@svipas Thanks (again) for your contribution 🚀
The issue has been added to the queue to be planned in for the next iterations!

svipas added 7 commits June 14, 2019 22:31

Add missing meta.brace.round scope

6ec692f

Rename nord.json to nord-color-theme.json

ed0e351

- It would show color palette, warnings/errors, etc. when the file is opened.

Remove editorActiveLineNumber.foreground

7f17611

Replace editorWidgetBorder

c5c6049

Replace editorGroup.background

cb28e21

Remove legacy support of notification

8661885

- Also, increase min. version of VS Code.

Revert: Add missing meta.brace.round scope

2fe22e8

svipas requested a review from svengreb as a code owner June 20, 2019 16:14

svipas mentioned this pull request Jun 20, 2019

editorWidget.border fixed #141

Merged

arcticicestudio self-requested a review July 20, 2019 15:35

arcticicestudio added context-ui context-workflow scope-compatibility type-improvement status-revision labels Jul 20, 2019

arcticicestudio assigned svipas Jul 20, 2019

arcticicestudio added the status-pending label Jul 20, 2019

arcticicestudio mentioned this pull request Jul 21, 2019

Rename theme file to enable theme development features #148

Closed

arcticicestudio approved these changes Jul 21, 2019 •

edited

Loading

View reviewed changes

arcticicestudio requested changes Jul 21, 2019

View reviewed changes

svengreb removed status-pending status-revision labels Feb 12, 2022

svengreb assigned arcticicestudio and svengreb and unassigned svipas Feb 12, 2022

svengreb unassigned arcticicestudio Aug 9, 2023

svengreb removed their assignment Aug 9, 2023

svengreb removed the type-improvement label Aug 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cosmetic changes #143

Cosmetic changes #143

svipas commented Jun 20, 2019 •

edited

Loading

arcticicestudio commented Jul 20, 2019 •

edited

Loading

arcticicestudio commented Jul 20, 2019

arcticicestudio Jul 21, 2019 •

edited

Loading

arcticicestudio Jul 21, 2019

arcticicestudio Jul 21, 2019

arcticicestudio Jul 21, 2019

arcticicestudio Jul 21, 2019

arcticicestudio Jul 21, 2019

arcticicestudio Jul 21, 2019

arcticicestudio left a comment

svipas commented Oct 4, 2019

svengreb commented Aug 9, 2023

-    "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",

-    "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",

Cosmetic changes #143

Are you sure you want to change the base?

Cosmetic changes #143

Conversation

svipas commented Jun 20, 2019 • edited Loading

arcticicestudio commented Jul 20, 2019 • edited Loading

arcticicestudio commented Jul 20, 2019

arcticicestudio Jul 21, 2019 • edited Loading

Choose a reason for hiding this comment

arcticicestudio Jul 21, 2019

Choose a reason for hiding this comment

arcticicestudio Jul 21, 2019

Choose a reason for hiding this comment

arcticicestudio Jul 21, 2019

Choose a reason for hiding this comment

arcticicestudio Jul 21, 2019

Choose a reason for hiding this comment

arcticicestudio Jul 21, 2019

Choose a reason for hiding this comment

arcticicestudio Jul 21, 2019

Choose a reason for hiding this comment

arcticicestudio left a comment

Choose a reason for hiding this comment

svipas commented Oct 4, 2019

svengreb commented Aug 9, 2023

svipas commented Jun 20, 2019 •

edited

Loading

arcticicestudio commented Jul 20, 2019 •

edited

Loading

arcticicestudio Jul 21, 2019 •

edited

Loading