From 5e0e362b3b6ad992ea07d39ef0e2e9c838d8f159 Mon Sep 17 00:00:00 2001
From: "Eyo O. Eyo" <7893459+eokoneyo@users.noreply.github.com>
Date: Thu, 19 Dec 2024 10:26:48 +0000
Subject: [PATCH] Resolve false positives with eslint no color rule (#204848)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
## Summary
Fixes error in lint rule that resulted in false positives, also added a
test case to ascertain the issue has been fixed. For context the error
happens in instances where specific CSS declarations that are
derivatives of shorthand declarations that can apply color to the HTML
element or text nodes where found, because the check we had simply
checked if we got a string back instead of asserting that it was a falsy
value.
## Before
## After
---
.../src/rules/no_css_color.test.ts | 39 +++++++++++++------
.../src/rules/no_css_color.ts | 8 ++--
2 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts
index e1f683b09814f..d5323e7423ce4 100644
--- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts
+++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts
@@ -45,7 +45,7 @@ const invalid: RuleTester.InvalidTestCase[] = [
filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx',
code: `
import React from 'react';
-
+
function TestComponent() {
return (
This is a test
@@ -86,7 +86,7 @@ const invalid: RuleTester.InvalidTestCase[] = [
filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx',
code: `
import React from 'react';
-
+
function TestComponent() {
const baseStyle = { background: 'rgb(255, 255, 255)' };
@@ -116,7 +116,7 @@ const invalid: RuleTester.InvalidTestCase[] = [
filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx',
code: `
import React from 'react';
-
+
function TestComponent() {
return (
This is a test
@@ -129,7 +129,7 @@ const invalid: RuleTester.InvalidTestCase[] = [
filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx',
code: `
import React from 'react';
-
+
function TestComponent() {
return (
This is a test
@@ -153,7 +153,7 @@ const invalid: RuleTester.InvalidTestCase[] = [
code: `
import React from 'react';
import { css } from '@emotion/css';
-
+
function TestComponent() {
return (
This is a test
@@ -171,7 +171,7 @@ const invalid: RuleTester.InvalidTestCase[] = [
const codeCss = css({
color: '#dd4040',
})
-
+
function TestComponent() {
return (
This is a test
@@ -187,7 +187,7 @@ const invalid: RuleTester.InvalidTestCase[] = [
import { css } from '@emotion/css';
const codeCss = css\` color: #dd4040; \`
-
+
function TestComponent() {
return (
This is a test
@@ -200,7 +200,7 @@ const invalid: RuleTester.InvalidTestCase[] = [
filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx',
code: `
import React from 'react';
-
+
function TestComponent() {
return (
({ color: '#dd4040' })}>This is a test
@@ -213,7 +213,7 @@ const invalid: RuleTester.InvalidTestCase[] = [
filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx',
code: `
import React from 'react';
-
+
function TestComponent() {
return (
This is a test
@@ -227,7 +227,7 @@ const invalid: RuleTester.InvalidTestCase[] = [
code: `
import React from 'react';
import { css } from '@emotion/css';
-
+
function TestComponent() {
return (
This is a test
@@ -237,7 +237,24 @@ const invalid: RuleTester.InvalidTestCase[] = [
},
];
-const valid: RuleTester.ValidTestCase[] = [];
+const valid: RuleTester.ValidTestCase[] = [
+ {
+ name: 'Does not raise an error when a CSS color is not used in a JSX css prop attribute',
+ filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx',
+ code: `
+ import React from 'react';
+ import { EuiCode } from '@elastic/eui';
+ import { css } from '@emotion/react';
+ function TestComponent() {
+ return (
+ This is a test
+ )
+ }`,
+ },
+];
for (const [name, tester] of [tsTester, babelTester]) {
describe(name, () => {
diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts
index c453e5edfcd74..fb73fe53fda07 100644
--- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts
+++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts
@@ -30,8 +30,8 @@ const checkPropertySpecifiesInvalidCSSColor = ([property, value]: string[]) => {
const style = new CSSStyleDeclaration();
- // @ts-ignore the types for this packages specifics an index signature of number, alongside other valid CSS properties
- style[property] = value;
+ // @ts-ignore the types for this packages specifies an index signature of number, alongside other valid CSS properties
+ style[property.trim()] = typeof value === 'string' ? value.trim() : value;
const anchor = propertiesSupportingCssColor.find((resolvedProperty) =>
property.includes(resolvedProperty)
@@ -42,9 +42,9 @@ const checkPropertySpecifiesInvalidCSSColor = ([property, value]: string[]) => {
// build the resolved color property to check if the value is a string after parsing the style declaration
const resolvedColorProperty = anchor === 'color' ? 'color' : anchor + 'Color';
- // in trying to keep this rule simple, it's enough if a string is used to define a color to mark it as invalid
+ // in trying to keep this rule simple, it's enough if we get a value back, because if it was an identifier we would have been able to set a value within this invocation
// @ts-ignore the types for this packages specifics an index signature of number, alongside other valid CSS properties
- return typeof style[resolvedColorProperty] === 'string';
+ return Boolean(style[resolvedColorProperty]);
};
const resolveMemberExpressionRoot = (node: TSESTree.MemberExpression): TSESTree.Identifier => {