Added support for 16-color and 1-color tty #29
Conversation
There was a problem hiding this comment.
Thanks for the PR @peetcamron! I've commented on a couple of small things that I think would be good to change but overall it looks great.
With regards to the comma's, I've intentionally left trailing comma's on objects. I find it just makes it easier when trying to add to them.
| }; | ||
|
|
||
| var theme = defaultThemesByColorLevel[colorSupportLevel]; | ||
| var colors = theme.palette; |
There was a problem hiding this comment.
Can the above (L6 - L15) be pulled into another module to import? Also, I think it would be clearer to use the supportsColor.has256, supportsColor.has16m, etc. rather than doing it by level.
There was a problem hiding this comment.
- Yes, this could be in its own little module
- I also prefer the has256 and has16m. The thing is that they are "independant" flags which doesn't fit very well the idea of having a single "supported color" field that we can compare, filter, sort... What if I turn it into constants instead? For instance:
COLORLEVEL_MONO, COLORLEVEL_16, COLORLEVEL_256, COLORLEVEL_16Mor any similar names. The level field in the themes could be turned into something likecolor: "mono" | "16" | "256" | "16m"so the theme designers would find it very easy to understand. Tell me your idea about this - For the commas, I can revert the change. I was not expecting ending up in a pull request :)
There was a problem hiding this comment.
I think that if the logic for choosing the color level were in its own module, the module should be able to just return the theme object. That way we don't have to deal with importing/choosing themes here (i.e. the module would just export what the colors variable is right now).
If you prefer to use constants, I think that's okay. I also think it would work to just use the independent flags right now similar to the first usage example they have on https://www.npmjs.com/package/supports-color. If people wanted to create themes, you would have to check what color level the terminal supports anyway.
|
|
||
| default: { | ||
| name: "Default color theme", | ||
| level: 3, // 16 milion colors |
|
@evanyeung you should really tighten the eslint rules on syntax to save some back and forth and keep things the way you like them. For instance, to keep the trailing comma you can add this rule: "comma-dangle": ["error", "never"] |
Hi,
I'm building myself a kind of terminal-only laptop and found out that most of the thing in your app wasn't showing in a basic tty because of the 16 color limitation. I added a very basic color theming to the app. I've put all of your colors in a default theme and created some other themes for the color limited tty. I have added the support-colors package to help me to auto-detect the caps of the terminal.
I also have fixed some invalid commas. If you don't want that, just tell me, I'll revert it and just keep the color theming.
Hope you will appreciate!
Thanks