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

refactor(tools): replace @colors/colors with smaller and faster ansis #4195

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

webdiscus
Copy link

This PR replaces @colors/colors used in CLI tools with smaller and faster ansis.

Benefits of ansis

Packages that already use ansis

facebook/stylex, nestjs/nest, sequelize/core, oclif/core, salesforcecli/cli and thousands others

Changes

The changes in CLI tools:

  • replace @colors/colors with ansis
  • add color preview for selectors used in CSS
  • fix ESLint errors in tools code

Checklist

  • Added markup tests, or they don't apply here because...
  • Updated the changelog at CHANGES.md

Test

This PR does not require unit/integration tests, but you can do the manual test.

Clone and install the forked repository:

git clone https://github.com/webdiscus/ansis-highlight.js.git
cd ansis-highlight.js
git checkout switch-to-ansis
npm i
npm run build

1) Run CheckTheme tools in CLI

For example, we want to check the vs2015 style:

node ./tools/checkTheme.js ./src/styles/vs2015.css

In console output will be displayed color text and NEW added preview of colors used in CSS selectors:
high fidelity highlighting (this is optional)

2) Run CheckAutoDetect tools in CLI

node ./tools/checkAutoDetect.js

In console output will be displayed color text:

Pasted Graphic

- replace @colors/colors with  ansis
- add color preview for selectors used in CSS
- fix ESLint errors in tools code
@@ -5,7 +5,7 @@ const css = require("css");
const wcagContrast = require("wcag-contrast");
const Table = require('cli-table');
const csscolors = require('css-color-names');
require("@colors/colors");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require("@colors/colors") <= extends the String.prototype, that is the bad practice.

@@ -5,7 +5,7 @@ const css = require("css");
const wcagContrast = require("wcag-contrast");
const Table = require('cli-table');
const csscolors = require('css-color-names');
require("@colors/colors");
const { cyan, green, yellow, magentaBright, hex } = require('ansis');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ansis supports the named import that allow to write shorter and cleaner code

const doesNotSupport = has_rules.map(x => x[1]).includes(false);
const skipped = has_rules.find(x => x[2]);
if (doesNotSupport || skipped) {
console.log(group.name.yellow);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

group.name is the string that prototype was extended with the yellow function.
This is unreadable and is very bad practice.

if (doesNotSupport) {
console.log(`- Theme does not fully support.`.brightMagenta);
console.log(magentaBright`- Theme does not fully support.`);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with ansis you can use the template string syntax very well,
this is clear and readable code unlike using extended String.prototype by @colors/colors

});
console.log();
}
}

const round2 = (x) => Math.round(x*100)/100;
const round2 = (x) => Math.round(x * 100) / 100;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix ESLint error


class CSSRule {
constructor(rule, body) {
this.rule = rule;
if (rule.declarations) {
this.bg = rule.declarations.find(x => x.property == "background-color")?.value;
this.bg = rule.declarations.find(x => x.property === "background-color")?.value;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix ESLint error

get background() {
return this.bg
return this.bg;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix ESLint error

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.

1 participant