-
-
Notifications
You must be signed in to change notification settings - Fork 33
Add 11 modern color scheme skins for MC #4775
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
base: master
Are you sure you want to change the base?
Add 11 modern color scheme skins for MC #4775
Conversation
3919e49
to
2df7167
Compare
Please remove *.po files from your commit. po-files are updated from Transifex. |
Add contemporary color schemes including Catppuccin (latte/mocha), Everforest, Gruvbox, Kanagawa, Nord, Rose Pine, Tokyo Night, and others. Update Makefile.am to include new skin files in distribution. Signed-off-by: Rafael Zago <[email protected]>
2df7167
to
83c0e96
Compare
Thanks for you review. Changes made under: 83c0e96 |
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.
First of all, thank you for the contribution! Cheers to Jindrich N. if you happen to work with him :)
I haven't tried these skins live, but I have checked them quickly for non-contrast colors with our new skin browser:
https://skins.midnight-commander.org
I would otherwise complain about the colors, because my understanding is that these are color schemes that are used like that in other software and people like them, so they should be taken as is. But invisible text somehow I think it better be addressed.
I think most skins are unproblematic, but I have found two exceptions. Could you please find a tweak that works?
catppuccin-latte

catppuccin

Also, a general question: you have added a test plan in the PR. Did you actually perform it?
[widget-editor] | ||
window-state-char = * | ||
window-close-char = X No newline at end of file |
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.
It seems that all the files are missing a trailing newline, whereas all our skins consistently have it. We don't have a linter for this, but I would appreciate it if you could fix it for consistency.
skindir = $(pkgdatadir)/skins | ||
|
||
skin_DATA = \ | ||
catppuccin.ini \ |
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.
By convention, all our 256-color skins are named something256.ini
to make it possible to have fallback non-256 versions and possibly boosted 16M versions of the same general scheme.
I think it would make sense to stick to this convention, and I would appreciate you renaming the files.
removed = color238;color210 | ||
error = color210;color248 | ||
|
||
[widget-panel] |
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.
Historically, it is so that our 256+ color skins use "fancy" characters for rendering (see, e.g. yadt256.ini
for an example of a "thin" skin - that is one not using the double lines) instead of normal terminal characters because it is assumed that if the terminal can display 256 colors, it also must be able to display UTF-8.
I think it would make sense to stick to this pattern for these color schemes unless you want to design something special for them.
What do you think?
Summary
Test plan
mc -S <skin-name>
New skins added