Skip to content
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

Theming #137

Closed
wants to merge 172 commits into from
Closed

Theming #137

wants to merge 172 commits into from

Conversation

phated
Copy link
Member

@phated phated commented Dec 10, 2017

@sttk @erikkemperman @tdeschryver I've begun experimenting with a solution to chalk replacement that utilizes yargs to parse --color/--no-color and replaces direct color names to work towards #63

There's still some problems with this approach so I may use @tdeschryver's solution of checking the process.argv directly for --no-colors

I just wanted to open this to get some more eyes on it.

@erikkemperman
Copy link
Member

erikkemperman commented Dec 13, 2017

@phated Sorry, I don't really have an opinion on this... Well, not an opinion that you don't already know, anyway :-) Having said that, it's probably just a typo above but is it --no-color or --no-colors?

@sttk
Copy link
Contributor

sttk commented Dec 13, 2017

This pr makes clear coloring and easy to add a function of configuring colors.

However some names are ambiguous, e.g. flag and desc. I think it's better to use property tree or consistent prefix for categorizing.

About --no-color(s), chalk supports flags: --no-color, --color=false, --no-colors for color-off, and flags: --color, --colors, --color=true, --color=always, and more flags for 256 or 16m colors for color-on. But I think it's OK to support only the flags: --color and --no-color which are announced in CLI document.

@phated
Copy link
Member Author

phated commented Dec 13, 2017

@sttk good points.

I was actually trying to add some more work onto this but ran into a problem because we register the logger more than once and need to clean it up. I'm starting to think the config-file stuff shouldn't be within liftoff but instead should be within the argv parser. Thoughts?

@phated phated changed the title Replace chalk Theming Dec 13, 2017
@sttk
Copy link
Contributor

sttk commented Dec 15, 2017

I've created a package: ansi-colors-nestable and ansi-colors-prioritized to extend ansi-colors. Are these usable?

To this pr, I tried to suggest about help message, such as:

var usage = `\n' +
  color.helpUsage('Usage:') + ' '
  color.helpSyntax('gulp ' + color.helpOptions('[options]') + ' tasks');

but ansi-color doesn't support nesting, and ' tasks' above is no color.

ansi-colors-nestable enables this as follows:

var nestableColor = require('ansi-colors-nestable');

color.helpSyntax = nestableColor('(color-name)');

var usage = `\n' +
  color.helpUsage('Usage:') + ' '
  color.helpSyntax('gulp ', color.helpOptions('[options]'), ' tasks');

Another one, ansi-colors-prioritized enables to set multiple colors with priority.
This would be useful for users who want to customize colors both in details and roughly.
For example:

var prioritizedColors = require('ansi-colors-prioritized');

var colorCfg = {
  ...
  require: {
    fail: '',
    moduleName: 'magenta',
  },
  system: {
    error: 'bgred',
  },
  ...
  moduleName: 'bold',
  error: 'red',
  ...
}

// ansiColors.bgred is used in this case.
color.requireFail = prioritizedColors(colorCfg.require.fail, colorCfg.system.error, colorCfg.error);

// ansiColors.bold is used in this case.
color.requiredModule = prioritizedColors(colorCfg.require.moduleName, colorCfg.moduleName);

log.error(
  color.requireFail('Failed to load external module ', color.requiredModule(name))
);

@phated
Copy link
Member Author

phated commented Dec 17, 2017

@sttk those are some interesting modules, but my thoughts are converging on using template strings

Something like:

var usage = '\n{helpUsage}Usage:{/helpUsage} {helpSyntax}gulp {helpOptions}[options]{/helpOptions} tasks{/helpSyntax}'

But I'm not happy with that verbosity and I'd like to solve argument replacement as well so we don't have to use string concatenation (probably using printf-style variables like %s)

@phated phated closed this Dec 21, 2017
@phated phated changed the base branch from master to pre-rebase December 21, 2017 20:41
@phated phated changed the base branch from pre-rebase to master December 21, 2017 20:42
@phated phated changed the base branch from master to pre-rebase December 21, 2017 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.