From 5fac62c7d51e4b42bfbccd0f1f012972f6483fab Mon Sep 17 00:00:00 2001 From: 1Copenut Date: Wed, 21 Sep 2022 16:45:33 -0500 Subject: [PATCH 1/7] WIP. First pass at CSS before spread child props ESLint rule. --- .../emotion_css_before_spread_props.js | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 scripts/eslint-plugin/emotion_css_before_spread_props.js diff --git a/scripts/eslint-plugin/emotion_css_before_spread_props.js b/scripts/eslint-plugin/emotion_css_before_spread_props.js new file mode 100644 index 00000000000..6c280a13b12 --- /dev/null +++ b/scripts/eslint-plugin/emotion_css_before_spread_props.js @@ -0,0 +1,55 @@ +module.exports = { + meta: { + type: 'problem', + docs: { + description: 'Enforce Emotion CSS props are declared before spread child props', + }, + }, + create: function(context) { + const attributesArr = []; + + return { + JSXOpeningElement(node) { + if(node.attributes) { + node.attributes.forEach(attribute => { + attributesArr.push(attribute); + }); + } + }, + 'Program:exit'() { + attributesArr.forEach((nodeObj, index) => { + if (isCssBeforeSpreadChildProps(nodeObj, index)) { + context.report({ + loc: { + line: nodeObj.loc.start.line, + column: nodeObj.loc.start.column + }, + message: 'CSS props must be declared before spread child props' + }); + } + }); + }, + }; + function isCssBeforeSpreadChildProps(nodeObj, index) { + const regex = new RegExp('^(\...)?[a-z]*[A-Z][A-z]*'); + let spreadChildProps; + + const cssPropsIndex = attributesArr.map(attribute => { + if (attribute.name && attribute.name.name) { + return attribute.name.name + } + }).indexOf('css'); + + const spreadChildPropsIndex = attributesArr.map(attribute => { + if (attribute.type === 'JSXSpreadAttribute' && attribute.argument.name.match(regex)) { + spreadChildProps = attribute.argument.name; + return spreadChildProps; + } + }).indexOf(spreadChildProps); + + if (cssPropsIndex && spreadChildPropsIndex && spreadChildPropsIndex === index) { + return cssPropsIndex > spreadChildPropsIndex + } + } + }, +}; From 1fced2440d6d272c0a530d798629e2910a3a9a05 Mon Sep 17 00:00:00 2001 From: 1Copenut Date: Thu, 22 Sep 2022 14:05:41 -0500 Subject: [PATCH 2/7] Refactoring CSS and spread child prop index calls. --- .../emotion_css_before_spread_props.js | 35 ++++++++----------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/scripts/eslint-plugin/emotion_css_before_spread_props.js b/scripts/eslint-plugin/emotion_css_before_spread_props.js index 6c280a13b12..810f4d66fbd 100644 --- a/scripts/eslint-plugin/emotion_css_before_spread_props.js +++ b/scripts/eslint-plugin/emotion_css_before_spread_props.js @@ -17,12 +17,12 @@ module.exports = { } }, 'Program:exit'() { - attributesArr.forEach((nodeObj, index) => { - if (isCssBeforeSpreadChildProps(nodeObj, index)) { + attributesArr.forEach((node, index) => { + if (!isCssBeforeSpreadChildProps(node, index)) { context.report({ loc: { - line: nodeObj.loc.start.line, - column: nodeObj.loc.start.column + line: node.loc.start.line, + column: node.loc.start.column }, message: 'CSS props must be declared before spread child props' }); @@ -30,26 +30,19 @@ module.exports = { }); }, }; - function isCssBeforeSpreadChildProps(nodeObj, index) { + function isCssBeforeSpreadChildProps(_, index) { const regex = new RegExp('^(\...)?[a-z]*[A-Z][A-z]*'); - let spreadChildProps; + const cssPropsIndex = attributesArr.findIndex(node => (node.name && node.name.name === 'css')); + const spreadChildPropsIndex = attributesArr.findIndex(node => (node.type === 'JSXSpreadAttribute' && node.argument.name.match(regex))); - const cssPropsIndex = attributesArr.map(attribute => { - if (attribute.name && attribute.name.name) { - return attribute.name.name - } - }).indexOf('css'); - - const spreadChildPropsIndex = attributesArr.map(attribute => { - if (attribute.type === 'JSXSpreadAttribute' && attribute.argument.name.match(regex)) { - spreadChildProps = attribute.argument.name; - return spreadChildProps; - } - }).indexOf(spreadChildProps); - - if (cssPropsIndex && spreadChildPropsIndex && spreadChildPropsIndex === index) { - return cssPropsIndex > spreadChildPropsIndex + // We're only checking if we have CSS props and spread child props + // The third check ensures the rule fires only on the spread child props line + if (cssPropsIndex && spreadChildPropsIndex && spreadChildPropsIndex === index) { + return cssPropsIndex < spreadChildPropsIndex } + + // Otherwise we don't have CSS props and/or spread child props + return true; } }, }; From 6f0edb766aa6cbd59f524bc5e5489675b83c8d1b Mon Sep 17 00:00:00 2001 From: 1Copenut Date: Thu, 22 Sep 2022 14:11:21 -0500 Subject: [PATCH 3/7] Improving the error reporting message. --- scripts/eslint-plugin/emotion_css_before_spread_props.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/eslint-plugin/emotion_css_before_spread_props.js b/scripts/eslint-plugin/emotion_css_before_spread_props.js index 810f4d66fbd..800ae428df8 100644 --- a/scripts/eslint-plugin/emotion_css_before_spread_props.js +++ b/scripts/eslint-plugin/emotion_css_before_spread_props.js @@ -24,7 +24,10 @@ module.exports = { line: node.loc.start.line, column: node.loc.start.column }, - message: 'CSS props must be declared before spread child props' + message: '{{ identifier }}: CSS props must be declared before spread child props', + data: { + identifier: node.argument.name + }, }); } }); From 9b8a2027893fd19b04d7693beac4b29a49ed1245 Mon Sep 17 00:00:00 2001 From: 1Copenut Date: Fri, 23 Sep 2022 13:22:39 -0500 Subject: [PATCH 4/7] Renaming file for clarity. Adding first passing unit tests. --- ...ps.js => css_before_spread_child_props.js} | 4 +- .../css_before_spread_child_props.test.js | 58 +++++++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) rename scripts/eslint-plugin/{emotion_css_before_spread_props.js => css_before_spread_child_props.js} (92%) create mode 100644 scripts/eslint-plugin/css_before_spread_child_props.test.js diff --git a/scripts/eslint-plugin/emotion_css_before_spread_props.js b/scripts/eslint-plugin/css_before_spread_child_props.js similarity index 92% rename from scripts/eslint-plugin/emotion_css_before_spread_props.js rename to scripts/eslint-plugin/css_before_spread_child_props.js index 800ae428df8..33ca9f6ac46 100644 --- a/scripts/eslint-plugin/emotion_css_before_spread_props.js +++ b/scripts/eslint-plugin/css_before_spread_child_props.js @@ -24,7 +24,7 @@ module.exports = { line: node.loc.start.line, column: node.loc.start.column }, - message: '{{ identifier }}: CSS props must be declared before spread child props', + message: '{{ identifier }}: CSS props must be declared before spread child props.', data: { identifier: node.argument.name }, @@ -40,7 +40,7 @@ module.exports = { // We're only checking if we have CSS props and spread child props // The third check ensures the rule fires only on the spread child props line - if (cssPropsIndex && spreadChildPropsIndex && spreadChildPropsIndex === index) { + if (cssPropsIndex >= 0 && spreadChildPropsIndex >= 0 && spreadChildPropsIndex === index) { return cssPropsIndex < spreadChildPropsIndex } diff --git a/scripts/eslint-plugin/css_before_spread_child_props.test.js b/scripts/eslint-plugin/css_before_spread_child_props.test.js new file mode 100644 index 00000000000..8a4df8e3829 --- /dev/null +++ b/scripts/eslint-plugin/css_before_spread_child_props.test.js @@ -0,0 +1,58 @@ +import rule from './css_before_spread_child_props.js'; +const RuleTester = require('eslint').RuleTester; + +const ruleTester = new RuleTester({ + parser: require.resolve('babel-eslint'), +}); + +const valid = [ + { + code: ` + {buttonIcon} + + `, + errors: [ + { + message: 'CSS props must be declared before spread child props', + }, + ], + }, +]; + +const invalid = [ + { + code: ` + {buttonIcon} + + `, + errors: [ + { + message: 'restLinkProps: CSS props must be declared before spread child props.', + }, + ], + }, +] + +ruleTester.run('css_before_spread_child_props', rule, { + valid, + invalid, +}); From 2aa1782721f999447d44c8ac9aca82a770cc423c Mon Sep 17 00:00:00 2001 From: 1Copenut Date: Mon, 26 Sep 2022 12:37:06 -0500 Subject: [PATCH 5/7] Focused on JSXElement, added unit tests, fixed failures in local linting." * Updating unit tests to pass before renaming. * Registering ESLint rule locally before adding unit tests. * Switched to JSXElement, and array of arrays. * Simplified logic, added unit tests, fixed failures. --- .eslintplugin.js | 1 + .eslintrc.js | 1 + .../css_before_spread_child_props.js | 51 ---- .../css_before_spread_child_props.test.js | 58 ----- .../eslint-plugin/css_before_spread_props.js | 47 ++++ .../css_before_spread_props.test.js | 245 ++++++++++++++++++ src/components/flyout/flyout_body.tsx | 2 +- src/components/flyout/flyout_footer.tsx | 2 +- src/components/flyout/flyout_header.tsx | 2 +- .../image/image_fullscreen_wrapper.tsx | 2 +- .../page/page_section/page_section.tsx | 2 +- src/components/pagination/pagination.tsx | 2 +- src/components/tool_tip/tool_tip_anchor.tsx | 2 +- src/services/theme/clone_element.test.tsx | 2 +- 14 files changed, 302 insertions(+), 117 deletions(-) delete mode 100644 scripts/eslint-plugin/css_before_spread_child_props.js delete mode 100644 scripts/eslint-plugin/css_before_spread_child_props.test.js create mode 100644 scripts/eslint-plugin/css_before_spread_props.js create mode 100644 scripts/eslint-plugin/css_before_spread_props.test.js diff --git a/.eslintplugin.js b/.eslintplugin.js index 50632e51018..348def29c45 100644 --- a/.eslintplugin.js +++ b/.eslintplugin.js @@ -4,4 +4,5 @@ exports.rules = { 'require-license-header': require('./scripts/eslint-plugin/require_license_header'), 'forward-ref': require('./scripts/eslint-plugin/forward_ref_display_name'), 'css-logical-properties': require('./scripts/eslint-plugin/css_logical_properties'), + 'css_before_spread_props': require('./scripts/eslint-plugin/css_before_spread_props'), }; diff --git a/.eslintrc.js b/.eslintrc.js index fd2ee1beca0..7b479ab617b 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -45,6 +45,7 @@ module.exports = { 'local/href-with-rel': 'error', 'local/forward-ref': 'error', 'local/css-logical-properties': 'error', + 'local/css_before_spread_props': 'error', 'local/require-license-header': [ 'warn', { diff --git a/scripts/eslint-plugin/css_before_spread_child_props.js b/scripts/eslint-plugin/css_before_spread_child_props.js deleted file mode 100644 index 33ca9f6ac46..00000000000 --- a/scripts/eslint-plugin/css_before_spread_child_props.js +++ /dev/null @@ -1,51 +0,0 @@ -module.exports = { - meta: { - type: 'problem', - docs: { - description: 'Enforce Emotion CSS props are declared before spread child props', - }, - }, - create: function(context) { - const attributesArr = []; - - return { - JSXOpeningElement(node) { - if(node.attributes) { - node.attributes.forEach(attribute => { - attributesArr.push(attribute); - }); - } - }, - 'Program:exit'() { - attributesArr.forEach((node, index) => { - if (!isCssBeforeSpreadChildProps(node, index)) { - context.report({ - loc: { - line: node.loc.start.line, - column: node.loc.start.column - }, - message: '{{ identifier }}: CSS props must be declared before spread child props.', - data: { - identifier: node.argument.name - }, - }); - } - }); - }, - }; - function isCssBeforeSpreadChildProps(_, index) { - const regex = new RegExp('^(\...)?[a-z]*[A-Z][A-z]*'); - const cssPropsIndex = attributesArr.findIndex(node => (node.name && node.name.name === 'css')); - const spreadChildPropsIndex = attributesArr.findIndex(node => (node.type === 'JSXSpreadAttribute' && node.argument.name.match(regex))); - - // We're only checking if we have CSS props and spread child props - // The third check ensures the rule fires only on the spread child props line - if (cssPropsIndex >= 0 && spreadChildPropsIndex >= 0 && spreadChildPropsIndex === index) { - return cssPropsIndex < spreadChildPropsIndex - } - - // Otherwise we don't have CSS props and/or spread child props - return true; - } - }, -}; diff --git a/scripts/eslint-plugin/css_before_spread_child_props.test.js b/scripts/eslint-plugin/css_before_spread_child_props.test.js deleted file mode 100644 index 8a4df8e3829..00000000000 --- a/scripts/eslint-plugin/css_before_spread_child_props.test.js +++ /dev/null @@ -1,58 +0,0 @@ -import rule from './css_before_spread_child_props.js'; -const RuleTester = require('eslint').RuleTester; - -const ruleTester = new RuleTester({ - parser: require.resolve('babel-eslint'), -}); - -const valid = [ - { - code: ` - {buttonIcon} - - `, - errors: [ - { - message: 'CSS props must be declared before spread child props', - }, - ], - }, -]; - -const invalid = [ - { - code: ` - {buttonIcon} - - `, - errors: [ - { - message: 'restLinkProps: CSS props must be declared before spread child props.', - }, - ], - }, -] - -ruleTester.run('css_before_spread_child_props', rule, { - valid, - invalid, -}); diff --git a/scripts/eslint-plugin/css_before_spread_props.js b/scripts/eslint-plugin/css_before_spread_props.js new file mode 100644 index 00000000000..c218f2b86ec --- /dev/null +++ b/scripts/eslint-plugin/css_before_spread_props.js @@ -0,0 +1,47 @@ +module.exports = { + meta: { + type: 'problem', + docs: { + description: 'Enforce CSS props are declared before spread props', + }, + }, + create: function(context) { + const allElementsArr = []; + + return { + JSXElement(node) { + const tempArr = []; + node.openingElement.attributes.forEach(attribute => tempArr.push(attribute)); + allElementsArr.push(tempArr); + }, + 'Program:exit'() { + allElementsArr.forEach(elementArr => { + const regex = new RegExp('^(\{\.\.\.).+\}$'); + const cssPropsIndex = elementArr.findIndex(node => (node.name && node.name.name === 'css')); + const spreadPropsIndex = elementArr.findIndex(node => (node.type === 'JSXSpreadAttribute')); + + if (cssPropsIndex === -1) { + return; + } + + if (spreadPropsIndex === -1) { + return; + } + + if (cssPropsIndex > spreadPropsIndex) { + context.report({ + loc: { + line: elementArr[cssPropsIndex].loc.start.line, + column: elementArr[cssPropsIndex].loc.start.column + }, + message: '{{ identifier }}: CSS props must be declared before spread props.', + data: { + identifier: elementArr[spreadPropsIndex].argument.name + }, + }); + } + }); + }, + }; + }, +}; diff --git a/scripts/eslint-plugin/css_before_spread_props.test.js b/scripts/eslint-plugin/css_before_spread_props.test.js new file mode 100644 index 00000000000..1df9860ebce --- /dev/null +++ b/scripts/eslint-plugin/css_before_spread_props.test.js @@ -0,0 +1,245 @@ +import rule from './css_before_spread_props.js'; +const RuleTester = require('eslint').RuleTester; + +const ruleTester = new RuleTester({ + parser: require.resolve('babel-eslint'), +}); + +const valid = [ + { + code: ` + {buttonIcon} + + `, + }, + { + code: ` + {buttonIcon} + + `, + }, + { + code: ` + {buttonIcon} + + `, + }, + { + code: ` + `, + }, + { + code: ` + `, + }, + { + code: ` + `, + }, + { + code: ` + {text} + + `, + }, + { + code: ` + Text string + + `, + }, +]; + +const invalid = [ + { + code: ` + {buttonIcon} + + `, + errors: [ + { + message: 'rest: CSS props must be declared before spread props.', + }, + ], + }, + { + code: ` + `, + errors: [ + { + message: 'rest: CSS props must be declared before spread props.', + }, + ], + }, + { + code: ` + `, + errors: [ + { + message: 'rest: CSS props must be declared before spread props.', + }, + ], + }, + { + code: ` + {text} + + `, + errors: [ + { + message: 'rest: CSS props must be declared before spread props.', + }, + ], + }, + { + code: ` + Text string + + `, + errors: [ + { + message: 'rest: CSS props must be declared before spread props.', + }, + ], + }, + { + code: `return !href && !onClick ? ( + + + {text} + + + ) : ( + + {text} + + ); + `, + errors: [ + { + message: 'restTextColor: CSS props must be declared before spread props.', + }, + { + message: 'restLink: CSS props must be declared before spread props.', + }, + ], + }, + { + code: `return !href && !onClick ? ( + + + {text} + + + ) : ( + + {text} + + ); + `, + errors: [ + { + message: 'restSpanProps: CSS props must be declared before spread props.', + }, + ], + }, +] + +ruleTester.run('css_before_spread_props', rule, { + valid, + invalid, +}); diff --git a/src/components/flyout/flyout_body.tsx b/src/components/flyout/flyout_body.tsx index 097231d3f3e..d17bf1274fc 100644 --- a/src/components/flyout/flyout_body.tsx +++ b/src/components/flyout/flyout_body.tsx @@ -42,7 +42,7 @@ export const EuiFlyoutBody: EuiFlyoutBodyProps = ({ ]; return ( -
+
+
{children}
); diff --git a/src/components/flyout/flyout_header.tsx b/src/components/flyout/flyout_header.tsx index c3833797036..9323b6e1ff7 100644 --- a/src/components/flyout/flyout_header.tsx +++ b/src/components/flyout/flyout_header.tsx @@ -33,7 +33,7 @@ export const EuiFlyoutHeader: EuiFlyoutHeaderProps = ({ const cssStyles = [styles.euiFlyoutHeader, hasBorder && styles.hasBorder]; return ( -
+
{children}
); diff --git a/src/components/image/image_fullscreen_wrapper.tsx b/src/components/image/image_fullscreen_wrapper.tsx index e31ed0d3960..b94fa02fe51 100644 --- a/src/components/image/image_fullscreen_wrapper.tsx +++ b/src/components/image/image_fullscreen_wrapper.tsx @@ -70,9 +70,9 @@ export const EuiImageFullScreenWrapper: FunctionComponent <>
= ({ return ( -
+
{children}
diff --git a/src/components/pagination/pagination.tsx b/src/components/pagination/pagination.tsx index a12622e2f3e..36e0bd2b718 100644 --- a/src/components/pagination/pagination.tsx +++ b/src/components/pagination/pagination.tsx @@ -292,8 +292,8 @@ export const EuiPagination: FunctionComponent = ({ centerPageCount = (
    {firstPageButtons} diff --git a/src/components/tool_tip/tool_tip_anchor.tsx b/src/components/tool_tip/tool_tip_anchor.tsx index 7429e3694c1..bc4708f73fa 100644 --- a/src/components/tool_tip/tool_tip_anchor.tsx +++ b/src/components/tool_tip/tool_tip_anchor.tsx @@ -56,8 +56,8 @@ export const EuiToolTipAnchor = forwardRef< // eslint-disable-next-line jsx-a11y/mouse-events-have-key-events { it('handles components', () => { const TestComponent: React.FC = (props) => ( -
    +
    hello world
    ); From 1427d1da7c1ddced9bb14e5e1f6aef616598c986 Mon Sep 17 00:00:00 2001 From: 1Copenut Date: Wed, 28 Sep 2022 14:08:39 -0500 Subject: [PATCH 6/7] Renamed temp arrays and refactored early returns for clarity. --- .../eslint-plugin/css_before_spread_props.js | 39 ++++++++++--------- src/components/pagination/pagination.tsx | 2 +- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/scripts/eslint-plugin/css_before_spread_props.js b/scripts/eslint-plugin/css_before_spread_props.js index c218f2b86ec..033695510a5 100644 --- a/scripts/eslint-plugin/css_before_spread_props.js +++ b/scripts/eslint-plugin/css_before_spread_props.js @@ -5,38 +5,39 @@ module.exports = { description: 'Enforce CSS props are declared before spread props', }, }, - create: function(context) { + create: function (context) { const allElementsArr = []; return { JSXElement(node) { - const tempArr = []; - node.openingElement.attributes.forEach(attribute => tempArr.push(attribute)); - allElementsArr.push(tempArr); + const attributesArr = []; + node.openingElement.attributes.forEach((attribute) => + attributesArr.push(attribute) + ); + allElementsArr.push(attributesArr); }, 'Program:exit'() { - allElementsArr.forEach(elementArr => { - const regex = new RegExp('^(\{\.\.\.).+\}$'); - const cssPropsIndex = elementArr.findIndex(node => (node.name && node.name.name === 'css')); - const spreadPropsIndex = elementArr.findIndex(node => (node.type === 'JSXSpreadAttribute')); + allElementsArr.forEach((elementArr) => { + const cssPropsIndex = elementArr.findIndex( + (node) => node.name && node.name.name === 'css' + ); + if (cssPropsIndex === -1) return; - if (cssPropsIndex === -1) { - return; - } - - if (spreadPropsIndex === -1) { - return; - } + const spreadPropsIndex = elementArr.findIndex( + (node) => node.type === 'JSXSpreadAttribute' + ); + if (spreadPropsIndex === -1) return; - if (cssPropsIndex > spreadPropsIndex) { + if (cssPropsIndex > spreadPropsIndex) { context.report({ loc: { line: elementArr[cssPropsIndex].loc.start.line, - column: elementArr[cssPropsIndex].loc.start.column + column: elementArr[cssPropsIndex].loc.start.column, }, - message: '{{ identifier }}: CSS props must be declared before spread props.', + message: + '{{ identifier }}: CSS props must be declared before spread props.', data: { - identifier: elementArr[spreadPropsIndex].argument.name + identifier: elementArr[spreadPropsIndex].argument.name, }, }); } diff --git a/src/components/pagination/pagination.tsx b/src/components/pagination/pagination.tsx index 36e0bd2b718..17332fc6fd0 100644 --- a/src/components/pagination/pagination.tsx +++ b/src/components/pagination/pagination.tsx @@ -292,9 +292,9 @@ export const EuiPagination: FunctionComponent = ({ centerPageCount = (
      {firstPageButtons} {selectablePages} From d74d8e5ac26226b8621ee6f2e676c256619160bd Mon Sep 17 00:00:00 2001 From: 1Copenut Date: Wed, 28 Sep 2022 14:39:00 -0500 Subject: [PATCH 7/7] Removing contentProps.css from cssContentStyles array. --- src/components/page/page_section/page_section.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/page/page_section/page_section.tsx b/src/components/page/page_section/page_section.tsx index 590755dc9cc..a3dfffadcbe 100644 --- a/src/components/page/page_section/page_section.tsx +++ b/src/components/page/page_section/page_section.tsx @@ -100,7 +100,6 @@ export const EuiPageSection: FunctionComponent = ({ bottomBorder === true && styles.border, alignment.toLowerCase().includes('center') && contentStyles.center, restrictWidth && contentStyles.restrictWidth, - contentProps?.css && contentProps.css, ]; return (