-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Theme fixes & added 2 new themes #972
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
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.
This is great! Thanks for the fixes, I left one comment.
packages/app/package.json
Outdated
@@ -205,6 +205,7 @@ | |||
"shortid": "^2.2.8", | |||
"store": "^2.0.12", | |||
"string-replace-loader": "^2.1.1", | |||
"strip-json-comments": "^2.0.1", |
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.
I don't think we need this as we already use JSON5 to parse (https://json5.org/). The reason I did that RegEx was because I wanted the default colors which are commented out to be used as well, as we don't have those default colors built in.
So if you have:
//"editorActiveLineNumber.foreground": "#0b216f",
I want it to become:
"editorActiveLineNumber.foreground": "#0b216f",
But only for those kind of comments, not for comments that explain something.
Maybe it works with these colors commented though, is worth testing.
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.
Yeah, seems like I completely misunderstood what that code did. 😃
Still, shouldn't that be
replace(/^\s*\/\/"/gm, '"')
or is JSON5 resilient with missing quotes in property names?
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're right! That's what it should be, big mistake on my side 😅.
@@ -43,13 +44,31 @@ function fetchTheme(foundTheme) { | |||
return foundTheme.get(); | |||
} | |||
|
|||
return window.fetch(foundTheme.url).then(x => x.json()); |
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.
Ahhh that makes a lot of sense!
packages/common/themes/index.js
Outdated
'https://github.com/raw/Microsoft/vscode/d2b6bbb46fbdf535e2c96b3e00ac56ac1d427a69/extensions/theme-solarized-light/themes/solarized-light-color-theme.json', | ||
}, | ||
{ | ||
name: 'Cobalt 2', |
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.
Could you move this one above VSCode Light
? That way we keep the Dark -> Light order 😄.
Theme fixes:
use strip-json-comments to... well, strip JSON comments 😃Themes added: