-
Notifications
You must be signed in to change notification settings - Fork 73
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
Enable themes #852
Enable themes #852
Conversation
…gnable to 'Color<ColorParam>'
…ENABLE_THEME was unset
c7027d2
to
653c1c2
Compare
Some tests are failing and I have not write any tests for my changes, but I'm ready for a review since the feature is mostly done and I need to know if there are bugs and your opinion on this. I have not tested few sections like: |
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.
@AllanOricil Great work 🏆
My biggest concern here is that users will have different terminal themes that would conflict with the configured theme. CLI developers would need to configure a theme that works on most terminal themes, which would be a challenging task.
I wonder if this should be something that we defer to users? Instead of looking in the CLI's package.json, oclif/core could look for a config file on the users machine. That would allow users to have colorful help without forcing them to use a specific terminal theme that works with it.
What do you think?
Works for me if I do this in
and this in
|
…ted using 'esModuleInterop'
fixed |
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.
Just a few small things and then I'll approve.
src/config/config.ts
Outdated
@@ -325,6 +328,11 @@ export class Config implements IConfig { | |||
|
|||
this.npmRegistry = this.scopedEnvVar('NPM_REGISTRY') || this.pjson.oclif.npmRegistry | |||
|
|||
const themeFilePath = resolve(this.configDir, 'theme.json') | |||
const theme = this.scopedEnvVarTrue('DISABLE_THEME') ? undefined : await safeReadJson(themeFilePath) |
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.
const theme = this.scopedEnvVarTrue('DISABLE_THEME') ? undefined : await safeReadJson<Record<string, string>>(themeFilePath)
That way you don't have to use as
when passing it to parseTheme
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.
fixed at 00a6ccd
Could you tell me if you agree with what I did. I removed enableTheme from config.
@AllanOricil I'm going to merge this into a separate feature branch so that we can run all the integration tests before merging. I'm taking a vacation tomorrow so I would expect to get this merged into |
@mdonnalley thank you for all the help 👍 |
* feat: create type for storing theme colors * feat: include color and @types/color, and simiplify theme schema * feat: set theme's default colors to white * feat: set FORCE_COLOR to 0||3 to follow the NO_COLOR manifest * feat: create DEFAULT_THEME to store default colors * revert: remove NO_COLOR manifest * feat: add new theme variables to style $, flag, and flag options * feat: add color to section headers * feat: configure default colors * feat: add colors for bin, command summary and version * feat: topics, commands, bin, version, sections, dollar sign are colorized * feat: configure default colors * feat: add feature flag to enabled/disable theme * feat: add colorize function to simplify the way colors are added to strings * feat: change all chalk.hex calls to colorize * feat: configure OCLIF_ENABLE_THEME to have precence over PJSON prop * feat: all theme colors are optional * fix: error TS2322: Type 'string' is not assignable to type 'boolean' * fix: error TS2345: arg of type 'Color<ColorParam>|undefined' not assignable to 'Color<ColorParam>' * fix: runtime error TypeError: color.hex is not a function * fix: this.pjson.oclif.enableTheme was not being evaluated when OCLIF_ENABLE_THEME was unset * chore(deps): update yarn.lock * refactor: simplified code removing OCLIF_ENABLE_THEME constant * fix: command summary was not changing its color when running the root command * feat: theme is now read from ~/config/<CLI>/theme.json if one exists * fix: add colors to other ARGUMENTS, EXAMPLES, DESCRIPTION, FLAGS DESCRIPTIONS sections * feat: add color token for aliases * fix(test): change template strings to avoid expected results printing bin twice * fix(test): add a missing parenthesis back so that all options are wraped by () * fix(test): remove single quotes wraping default flag values * feat: add test:debug script to ease debuging * refactor: add a constant with all possible THEME_KEYS * test: add tests to prove parsing untyped json object with color strings work * chore(package): add new scripts to ease development while writing tests * revert: remove theme from PJSON because a theme will be loaded from config_dir/<CLI>/theme.json * feat: add scopedEnvVarBoolean because scopedEnvVarTrue does not consider unset env vars * feat(test): add tests to prove the behavior that enables theme * refactor: replace all scopedEnvVarTrue by scopedEnvVarBoolean, which considers unset env variables * test: add tests to prove the enableTheme behavior * refactor: simplify parseTheme method * chore(package): remove localhost:4873 from lock file * revert: rever scopedEnvVarBoolean back to scopedEnvVarTrue * test: add tests to prove when this.theme is set * feat: ensure colorize returns string only * test: add tests to prove colorize works as expected * revert: rollback scopedEnvVarTrue as it was before * refactor: move parseTheme to src/util/util.js * refactor: make Theme type dinamically based on the values of THEME_KEYS at runtime * fix: err TS1259: Module /@types/color/index can only be default-imported using 'esModuleInterop' * revert: remove enableTheme from pjson because we don't want to cli devs to force users to use theme * revert: remove undefined as a return type for scopedEnvVarTrue * refactor: remove config.enableTheme Co-authored-by: Allan Oricil <[email protected]>
With this feature developers will be able to create custom color themes for their CLI.
obs: disregard the
--json
I added in the pictures belowBEFORE
AFTER
A theme is enabled when:
~/.config/data/<CLI>/theme.json
<CLI>_DISABLE_THEME
env var is not setAll theme colors are optional
LIGHT THEME EXAMPLES
Configurable Tokens
Colors are set with the values from
~/.config/<CLI>/theme.json
file, if one is present. Colors can be set with Hex and RGB color codes strings, which are converted to colors using the color package.In the following pictures you can understand the regions each token affect.
alias
bin
command
commandSummary
dollarSign
flag
flagDefaultValue
flagOptions
flagRequired
flagSeparator
flagType
NOT IMPLEMENTED YET.
It depends on a new attribute called "typeLabel" which has to be added to the flag configuration. Without allowing CLI developers to specify a custom label for the types of their flags, like
filePath
, users would only see "boolean | option" which is the value fromflag.type
.sectionDescription
sectionHeader
topic
version