Skip to content

Commit

Permalink
chore: add a new eslint rule prevent using margin (#17024)
Browse files Browse the repository at this point in the history
* chore: add a new eslint rule prevent using margin
  • Loading branch information
koji authored Dec 20, 2024
1 parent 47dbbe9 commit 3f10621
Show file tree
Hide file tree
Showing 6 changed files with 131 additions and 34 deletions.
19 changes: 19 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -180,13 +180,32 @@ module.exports = {
files: ['./protocol-designer/src/**/*.@(ts|tsx)'],
rules: {
'opentrons/no-imports-up-the-tree-of-life': 'warn',
'opentrons/no-margins-in-css': 'warn',
'opentrons/no-margins-inline': 'warn',
},
},
// apply application structure import requirements to app
{
files: ['./app/src/**/*.@(ts|tsx)'],
rules: {
'opentrons/no-imports-across-applications': 'error',
'opentrons/no-margins-in-css': 'warn',
'opentrons/no-margins-inline': 'warn',
},
},
{
files: ['./opentrons-ai-client/src/**/*.@(ts|tsx)'],
rules: {
'opentrons/no-imports-up-the-tree-of-life': 'warn',
'opentrons/no-margins-in-css': 'warn',
'opentrons/no-margins-inline': 'warn',
},
},
{
files: ['./components/src/**/*.@(ts|tsx)'],
rules: {
'opentrons/no-margins-in-css': 'warn',
'opentrons/no-margins-inline': 'warn',
},
},
],
Expand Down
44 changes: 11 additions & 33 deletions app/src/molecules/Command/CommandText.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type * as React from 'react'
import { pick } from 'lodash'
import { css } from 'styled-components'

import {
ALIGN_CENTER,
Expand Down Expand Up @@ -174,21 +175,10 @@ function ThermocyclerRunProfile(
>
<ul>
{shouldPropagateTextLimit(propagateTextLimit, isOnDevice) ? (
<li
css={`
margin-left: ${SPACING.spacing4};
`}
>
{stepTexts[0]}
</li>
<li css={LIST_STYLE}>{stepTexts[0]}</li>
) : (
stepTexts.map((step: string, index: number) => (
<li
css={`
margin-left: ${SPACING.spacing4};
`}
key={index}
>
<li css={LIST_STYLE} key={index}>
{' '}
{step}
</li>
Expand Down Expand Up @@ -252,42 +242,26 @@ function ThermocyclerRunExtendedProfile(
>
<ul>
{shouldPropagateTextLimit(propagateTextLimit, isOnDevice) ? (
<li
css={`
margin-left: ${SPACING.spacing4};
`}
>
<li css={LIST_STYLE}>
{profileElementTexts[0].kind === 'step'
? profileElementTexts[0].stepText
: profileElementTexts[0].cycleText}
</li>
) : (
profileElementTexts.map((element, index: number) =>
element.kind === 'step' ? (
<li
css={`
margin-left: ${SPACING.spacing4};
`}
key={`tc-outer-step-${index}`}
>
<li css={LIST_STYLE} key={`tc-outer-step-${index}`}>
{' '}
{element.stepText}
</li>
) : (
<li
css={`
margin-left: ${SPACING.spacing4};
`}
key={`tc-outer-step-${index}`}
>
<li css={LIST_STYLE} key={`tc-outer-step-${index}`}>
{element.cycleText}
<ul>
{element.stepTexts.map(
({ stepText }, stepIndex: number) => (
<li
css={`
margin-left: ${SPACING.spacing8};
`}
css={LIST_STYLE}
key={`tc-inner-step-${index}.${stepIndex}`}
>
{' '}
Expand All @@ -305,3 +279,7 @@ function ThermocyclerRunExtendedProfile(
</Flex>
)
}

const LIST_STYLE = css`
margin-left: ${SPACING.spacing4};
`
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ import {
Flex,
Icon,
JUSTIFY_SPACE_BETWEEN,
LegacyStyledText,
Link,
SPACING_AUTO,
SPACING,
LegacyStyledText,
TYPOGRAPHY,
} from '@opentrons/components'

Expand Down
2 changes: 2 additions & 0 deletions scripts/eslint-plugin-opentrons/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,6 @@
module.exports.rules = {
'no-imports-up-the-tree-of-life': require('./rules/no-imports-up-the-tree-of-life'),
'no-imports-across-applications': require('./rules/no-imports-across-applications'),
'no-margins-in-css': require('./rules/no-margins-in-css'),
'no-margins-inline': require('./rules/no-margins-inline'),
}
46 changes: 46 additions & 0 deletions scripts/eslint-plugin-opentrons/lib/rules/no-margins-in-css.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
module.exports = {
meta: {
type: 'problem',
docs: {
description: 'Disallow the use of margin-related properties in css-in-js',
category: 'Best Practices',
recommended: false,
},
messages: {
noMarginInCssInJs: "Avoid using '{{property}}' in css-in-js.",
},
},
create(context) {
return {
// Check for CSS-in-JS template literals
TaggedTemplateExpression(node) {
const forbiddenMargins = [
'margin',
// 'margin-top',
// 'margin-left',
// 'margin-right',
// 'margin-bottom',
]

if (node.tag.type === 'Identifier' && node.tag.name === 'css') {
const templateLiteral = node.quasi
templateLiteral.quasis.forEach(quasi => {
const text = quasi.value.raw
forbiddenMargins.forEach(property => {
const regex = new RegExp(`\\b${property}\\b`, 'i')
if (regex.test(text)) {
context.report({
node: quasi,
messageId: 'noMarginInCssInJs',
data: {
property,
},
})
}
})
})
}
},
}
},
}
52 changes: 52 additions & 0 deletions scripts/eslint-plugin-opentrons/lib/rules/no-margins-inline.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
module.exports = {
meta: {
type: 'problem',
docs: {
description: 'Disallow the use of margin-related styles',
category: 'Best Practices',
recommended: false,
},
messages: {
noMarginInline: "Avoid using '{{property}}' in your components.",
},
schema: [],
},
create(context) {
const forbiddenMargins = [
'margin',
'marginLeft',
'marginRight',
'marginTop',
'marginBottom',
'marginX',
'marginY',
]

return {
// Existing visitor for object properties
Property(node) {
if (forbiddenMargins.includes(node.key.name || node.key.value)) {
context.report({
node: node.key,
messageId: 'noMarginInline',
data: {
property: node.key.name || node.key.value,
},
})
}
},
// New visitor for JSX attributes
JSXAttribute(node) {
if (forbiddenMargins.includes(node.name.name)) {
context.report({
node: node.name,
messageId: 'noMarginInline',
data: {
property: node.name.name,
},
})
}
},
}
},
}

0 comments on commit 3f10621

Please sign in to comment.