-
Notifications
You must be signed in to change notification settings - Fork 14k
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
chore: Eslint custom plugin to warn about hex and literal colors #19239
Conversation
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.
Thanks for adding this plugin, I left a few comments. I have tried it and it works fine, but there is a limitation that it isn't able to check the color in Styled. Is it expected?
Yes, that's the current limitation that I am trying to resolve. Thanks for your comments! |
Codecov Report
@@ Coverage Diff @@
## master #19239 +/- ##
=======================================
Coverage 66.48% 66.48%
=======================================
Files 1670 1670
Lines 63952 63952
Branches 6506 6506
=======================================
Hits 42519 42519
Misses 19747 19747
Partials 1686 1686
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
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.
I'm worried about the execution time. Even with the suggestion to improve hasLiteralColor
the time is still high. Here's npm run _lint
execution times (in minutes:seconds):
With rule disabled: 1:44
With original rule enabled: 27:41
With code suggestions: 25:54
I also found a false positive in src/visualizations/TimeTable/TimeTable.jsx
.
Maybe we can invest some time to make it faster?
superset-frontend/buildtools/eslint-plugin-theme-colors/index.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
I am looking into the performance and see what we can do. As for the false-positive in
|
Oh, I missed that! Thanks! |
superset-frontend/buildtools/eslint-plugin-theme-colors/index.js
Outdated
Show resolved
Hide resolved
@geido Wow nice improvements! Really reduced the execution time. One way we can test it is to compare the output with the previous version. |
Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
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.
LGTM. Thank you for the awesome tool. It will really help our efforts to make Superset themeable.
) * wip * Add eslint custom plugin * Refactor * Clean up * Update superset-frontend/buildtools/eslint-plugin-theme-colors/index.js Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * Refactor * Update superset-frontend/buildtools/eslint-plugin-theme-colors/index.js Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * Clean up Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> (cherry picked from commit 6b9113a)
…che#19239) * wip * Add eslint custom plugin * Refactor * Clean up * Update superset-frontend/buildtools/eslint-plugin-theme-colors/index.js Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * Refactor * Update superset-frontend/buildtools/eslint-plugin-theme-colors/index.js Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * Clean up Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
SUMMARY
Warns about the usage of rgba, hex and literal colors over the preferred theme variables.
TESTING INSTRUCTIONS
npm run _lint
in superset-frontendADDITIONAL INFORMATION