-
Notifications
You must be signed in to change notification settings - Fork 265
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
Update themes #377
Update themes #377
Conversation
This pretty prints the theme file rather than compressing it on to a single line. This makes it easier to see what has changed between versions.
Change the update source to the source to the same one that windowsterminalthemes.dev uses rather than the backup theme file which is only used if that file cannot be loaded. https://github.com/atomcorp/themes/blob/master/app/src/App.tsx#L18 Filter out the cursor, selection and credits fields from the source file to make the diff easier to grok (added back in the following commit). BREAKING CHANGES: - Gruvbox Dark renamed to GruvboxDark. - Monokai Octagon renamed to Monokai Pro (Filter Octagon)
In the previous commit, the cursor and selection colors were removed, as well as the credits field to make the commit easier to view. This commit adds them back.
@caarlos0 I'm okay with merging this but my gut tells me we probably want to keep the JSON file minified for binary size purposes. |
A quick check suggests that the built file size isn't at all dominated by the whitespace changes (on a Mac at least). I'm not super familiar with go executable slimming, but 140KB seems reasonable to me regardless of size constraints.
(Edited as I inadvertently had the changes in my local main branch) |
The themes which were in themes_custom.json are now in the main themes.json file. This commit removes the themes_custom.json file, and updates the Makefile and THEMES.md file to reflect this change.
In the latest push I fixed up the tests by removing the themes_custom.json which was full of duplicate themes from themes.json. I manually checked all these - all were exact copies (except sublette, which had short hex colors like Removed the code that loaded both theme files and fixed the theme count test. |
I think it wouldn't hurt to add other than that, lgtm |
The pros to minifying don't outweigh the cons here. Storing the file unformatted makes it difficult to see the effect of any future PR and makes it difficult for developers to read and work with the json. If we want to minify it, that would be best to do as part of the build, but even then I'd suggest only doing this if there's some measurable benefit in speed or size. Here the size difference is < 1%, and although I haven't checked the perf difference, it's likely similarly negligible (loading 100kb to RAM from a local file is pretty fast). |
Thanks @joshka, yeah I agree it's worth having the mini-fication on build (and keeping unminified in source). We can likely achieve this through |
This is 3 commits intentionally to make this PR easier to review:
commit f087371
Author: Josh McKinney [email protected]
Date: Tue Oct 3 01:35:01 2023 -0700
commit 6059dfb
Author: Josh McKinney [email protected]
Date: Tue Oct 3 02:10:52 2023 -0700
commit 28e710f
Author: Josh McKinney [email protected]
Date: Tue Oct 3 02:18:21 2023 -0700