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.

Sign up for GitHub

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

ESLint rule to validate css props are declared before{...spread} props #6257

Merged
merged 7 commits into from
Sep 28, 2022
Merged

ESLint rule to validate css props are declared before{...spread} props #6257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5fac62c
WIP. First pass at CSS before spread child props ESLint rule.
1Copenut Sep 21, 2022
1fced24
Refactoring CSS and spread child prop index calls.
1Copenut Sep 22, 2022
6f0edb7
Improving the error reporting message.
1Copenut Sep 22, 2022
9b8a202
Renaming file for clarity. Adding first passing unit tests.
1Copenut Sep 23, 2022
2aa1782
Focused on JSXElement, added unit tests, fixed failures in local lint…
1Copenut Sep 26, 2022
1427d1d
Renamed temp arrays and refactored early returns for clarity.
1Copenut Sep 28, 2022
d74d8e5
Removing contentProps.css from cssContentStyles array.
1Copenut Sep 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintplugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
};
1 change: 1 addition & 0 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
{
Expand Down
48 changes: 48 additions & 0 deletions scripts/eslint-plugin/css_before_spread_props.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
module.exports = {
meta: {
type: 'problem',
docs: {
description: 'Enforce CSS props are declared before spread props',
},
},
create: function (context) {
const allElementsArr = [];

return {
JSXElement(node) {
const attributesArr = [];
node.openingElement.attributes.forEach((attribute) =>
attributesArr.push(attribute)
);
allElementsArr.push(attributesArr);
},
'Program:exit'() {
allElementsArr.forEach((elementArr) => {
const cssPropsIndex = elementArr.findIndex(
(node) => node.name && node.name.name === 'css'
);
if (cssPropsIndex === -1) return;

const spreadPropsIndex = elementArr.findIndex(
(node) => node.type === 'JSXSpreadAttribute'
);
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,
},
});
}
});
},
};
},
};
245 changes: 245 additions & 0 deletions scripts/eslint-plugin/css_before_spread_props.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 = [
1Copenut marked this conversation as resolved.
Show resolved Hide resolved
{
code: `<a
css={cssStyles}
{...rest}
>
{buttonIcon}
</a>
`,
},
{
code: `<a css={cssStyles}>
{buttonIcon}
</a>
`,
},
{
code: `<a {...rest}>
{buttonIcon}
</a>
`,
},
{
code: `<nav {...rest}>
<ol
className="euiBreadcrumbs__list"
css={cssBreadcrumbsListStyles}
>
{breadcrumbChildren}
</ol>
</nav>
`,
},
{
code: `<nav {...rest}>
<ol css={cssBreadcrumbsListStyles}>
{breadcrumbChildren}
</ol>
</nav>
`,
},
{
code: `<nav {...rest}>
<ol {...rest}>
{breadcrumbChildren}
</ol>
</nav>
`,
},
{
code: `<EuiLink
ref={ref}
css={cssStyles}
{...rest}
>
{text}
</EuiLink>
`,
},
{
code: `<EuiLink
ref={ref}
css={cssStyles}
{...rest}
>
Text string
</EuiLink>
`,
},
];

const invalid = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each invalid block was built out from real failures in EUI components, or derived therefrom.

{
code: `<a
{...rest}
css={cssStyles}
>
{buttonIcon}
</a>
`,
errors: [
{
message: 'rest: CSS props must be declared before spread props.',
},
],
},
{
code: `<nav {...rest}>
<ol
{...rest}
css={cssBreadcrumbsListStyles}
>
{breadcrumbChildren}
</ol>
</nav>
`,
errors: [
{
message: 'rest: CSS props must be declared before spread props.',
},
],
},
{
code: `<nav>
<ol
{...rest}
css={cssBreadcrumbsListStyles}
>
{breadcrumbChildren}
</ol>
</nav>
`,
errors: [
{
message: 'rest: CSS props must be declared before spread props.',
},
],
},
{
code: `<EuiLink
ref={ref}
{...rest}
css={cssStyles}
>
{text}
</EuiLink>
`,
errors: [
{
message: 'rest: CSS props must be declared before spread props.',
},
],
},
{
code: `<EuiLink
ref={ref}
{...rest}
css={cssStyles}
>
Text string
</EuiLink>
`,
errors: [
{
message: 'rest: CSS props must be declared before spread props.',
},
],
},
{
code: `return !href && !onClick ? (
<EuiTextColor
color={highlightLastBreadcrumb ? 'default' : 'subdued'}
cloneElement
{...restTextColor}
css={cssStyles}
>
<span
ref={ref}
title={title}
aria-current={ariaCurrent}
className={classes}
css={cssStyles}
>
{text}
</span>
</EuiTextColor>
) : (
<EuiLink
{...restLink}
ref={ref}
title={title}
aria-current={ariaCurrent}
className={classes}
css={cssStyles}
color={color || (highlightLastBreadcrumb ? 'text' : 'subdued')}
onClick={onClick}
href={href}
rel={rel}
>
{text}
</EuiLink>
);
`,
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 ? (
<EuiTextColor
color={highlightLastBreadcrumb ? 'default' : 'subdued'}
cloneElement
css={cssStyles}
>
<span
ref={ref}
title={title}
aria-current={ariaCurrent}
className={classes}
{...restSpanProps}
css={cssStyles}
>
{text}
</span>
</EuiTextColor>
) : (
<EuiLink
ref={ref}
title={title}
aria-current={ariaCurrent}
className={classes}
css={cssStyles}
color={color || (highlightLastBreadcrumb ? 'text' : 'subdued')}
onClick={onClick}
href={href}
rel={rel}
>
{text}
</EuiLink>
);
`,
errors: [
{
message: 'restSpanProps: CSS props must be declared before spread props.',
},
],
},
]

ruleTester.run('css_before_spread_props', rule, {
valid,
invalid,
});
2 changes: 1 addition & 1 deletion src/components/flyout/flyout_body.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export const EuiFlyoutBody: EuiFlyoutBodyProps = ({
];

return (
<div className={classes} {...rest} css={cssStyles}>
<div className={classes} css={cssStyles} {...rest}>
<div
tabIndex={0}
className="euiFlyoutBody__overflow"
Expand Down
2 changes: 1 addition & 1 deletion src/components/flyout/flyout_footer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export const EuiFlyoutFooter: EuiFlyoutFooterProps = ({
const cssStyles = [styles.euiFlyoutFooter];

return (
<div className={classes} {...rest} css={cssStyles}>
<div className={classes} css={cssStyles} {...rest}>
{children}
</div>
);
Expand Down
2 changes: 1 addition & 1 deletion src/components/flyout/flyout_header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export const EuiFlyoutHeader: EuiFlyoutHeaderProps = ({
const cssStyles = [styles.euiFlyoutHeader, hasBorder && styles.hasBorder];

return (
<div className={classes} {...rest} css={cssStyles}>
<div className={classes} css={cssStyles} {...rest}>
{children}
</div>
);
Expand Down
2 changes: 1 addition & 1 deletion src/components/image/image_fullscreen_wrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,9 @@ export const EuiImageFullScreenWrapper: FunctionComponent<EuiImageWrapperProps>
<>
<figure
aria-label={optionalCaptionText}
css={cssStyles}
{...wrapperProps}
className={classes}
css={cssStyles}
>
<EuiImageButton
hasAlt={!!alt}
Expand Down
3 changes: 1 addition & 2 deletions src/components/page/page_section/page_section.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,11 @@ export const EuiPageSection: FunctionComponent<EuiPageSectionProps> = ({
bottomBorder === true && styles.border,
alignment.toLowerCase().includes('center') && contentStyles.center,
restrictWidth && contentStyles.restrictWidth,
contentProps?.css && contentProps.css,
];

return (
<Component css={cssStyles} {...rest}>
<div {...contentProps} css={cssContentStyles} style={widthStyles}>
<div css={cssContentStyles} {...contentProps} style={widthStyles}>
1Copenut marked this conversation as resolved.
Show resolved Hide resolved
{children}
</div>
</Component>
Expand Down
4 changes: 2 additions & 2 deletions src/components/pagination/pagination.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,9 +292,9 @@ export const EuiPagination: FunctionComponent<Props> = ({

centerPageCount = (
<ul
{...accessibleName}
css={paginationStyles.euiPagination__list}
className="euiPagination__list"
css={paginationStyles.euiPagination__list}
{...accessibleName}
>
{firstPageButtons}
{selectablePages}
Expand Down
2 changes: 1 addition & 1 deletion src/components/tool_tip/tool_tip_anchor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ export const EuiToolTipAnchor = forwardRef<
// eslint-disable-next-line jsx-a11y/mouse-events-have-key-events
<span
ref={ref}
{...rest}
css={cssStyles}
{...rest}
className={classes}
onMouseOver={onMouseOver}
onMouseOut={onMouseOut}
Expand Down
2 changes: 1 addition & 1 deletion src/services/theme/clone_element.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ describe('cloneElementWithCss', () => {

it('handles components', () => {
const TestComponent: React.FC = (props) => (
<div {...props} css={{ backgroundColor: 'blue' }}>
<div css={{ backgroundColor: 'blue' }} {...props}>
1Copenut marked this conversation as resolved.
Show resolved Hide resolved
hello world
</div>
);
Expand Down