Skip to content

Commit

Permalink
Extend shorthand-property-sorting ESLint rule to handle style composi…
Browse files Browse the repository at this point in the history
…tion (#1749)

* Rewrite rule to handle almost all Compiled APIs

* Improve rule

* Prevent borderTop and borderBottom from being treated as overlapping properties

* Add changeset

* Update eslint-plugin README description

* Remove most cssMap false positives, add more tests
  • Loading branch information
dddlr authored Dec 9, 2024
1 parent f63b99d commit 88bbe38
Show file tree
Hide file tree
Showing 9 changed files with 1,394 additions and 309 deletions.
5 changes: 5 additions & 0 deletions .changeset/fuzzy-readers-pump.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@compiled/utils': patch
---

Remove superfluous border-block-_ and border-inline-_ from being listed as shorthand properties of border-top / border-bottom / border-left / border-right
5 changes: 5 additions & 0 deletions .changeset/twelve-countries-fail.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@compiled/eslint-plugin': minor
---

Fix shorthand-property-sorting not detecting lint violations, and extend the rule to support almost all Compiled APIs
2 changes: 1 addition & 1 deletion packages/eslint-plugin/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,4 +79,4 @@ You can also enable the recommended rules for compiled by adding `plugin:@compil
| [@compiled/no-keyframes-tagged-template-expression](./src/rules/no-keyframes-tagged-template-expression) | Disallows the `keyframes` tagged template expression || 🔧 |
| [@compiled/no-styled-tagged-template-expression](./src/rules/no-styled-tagged-template-expression) | Disallows the `styled` tagged template expression || 🔧 |
| [@compiled/no-suppress-xcss](./src/rules/no-suppress-xcss) | The xcss prop is predicated on adhering to the type contract. Supressing it breaks this contract and thus is not allowed. || |
| [@compiled/shorthand-property-sorting](./src/rules/shorthand-property-sorting) | Prevent unwanted side-effect by ensuring shorthand properties are always defined before their related longhands. || |
| [@compiled/shorthand-property-sorting](./src/rules/shorthand-property-sorting) | Prevent unwanted side-effects by ensuring shorthand properties are always defined before their corresponding longhand properties. || |
4 changes: 3 additions & 1 deletion packages/eslint-plugin/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@
"src"
],
"dependencies": {
"@compiled/utils": "^0.13.0"
"@compiled/utils": "^0.13.0",
"estraverse": "^5.3.0"
},
"devDependencies": {
"@babel/eslint-parser": "^7.21.8",
"@types/estraverse": "^5.1.7",
"@types/estree": "^1.0.3",
"@types/estree-jsx": "^1.0.2",
"@typescript-eslint/parser": "^5.59.8",
Expand Down
128 changes: 124 additions & 4 deletions packages/eslint-plugin/src/rules/shorthand-property-sorting/README.md
Original file line number Diff line number Diff line change
@@ -1,12 +1,127 @@
# `shorthand-property-sorting`

Enforces css property sorting for packages `css`, `cssMap`, `styled` that originates from `@compiled/react`, and `@atlaskit/css`.

Having a longhand (like `fontSize` and `borderTopColor`) before a shorthand (like `font` and `border`) is not valid CSS. While this is not usually a problem, it becomes a problem in Compiled where classes are generated for each CSS declaration and rules are then sorted at build time during stylesheet extraction. As a result, this could cause unwanted side effects when CSS longhands are redudant.
Enforces CSS property sorting for packages `css`, `cssMap`, `styled` that originates from `@compiled/react`, and `@atlaskit/css`.

This rule enforces that the order in which the properties appear in a component's source code matches the actual ordering the properties will have at build time and runtime.

## Rule details
## Context

CSS has "shorthand properties", like `font` and `border`, that are useful shorthands for other, more verbose properties. For example, `font` is a shorthan property for `fontSize`, `fontWeight`, `lineHeight` (surprisingly enough!), and so on.

When you mix shorthand properties and non-shorthand properties, Compiled sorts these CSS properties in a specific way. If you write your styles in a way that doesn't match the way that Compiled sorts them at build-time and runtime, this ESLint rule will alert you.

Let's take some examples.

The following example implies the `paddingTop` will be overridden by `padding`. However, Compiled will actually sort the `padding` so that it gets overridden by `paddingTop` instead.

```tsx
import { css } from '@compiled/react';

const styles = css({
// ESLint violation: padding should come before paddingTop
paddingTop: token('space.050'),
padding: token('space.100'),
});

const Component = ({ children }) => {
return <div css={styles}>{children}</div>;
};
```

And in the following example, the code implies that `borderColor` will be overridden by `borderTop`, because `borderTop` actually implicitly sets `borderColor` to `initial`. Compiled doesn't do this -- `borderTop` will come before `borderColor` instead.

```tsx
import { css } from '@compiled/react';

const styles = css({
// ESLint violation: borderTop should come before borderColor
borderColor: 'blue',
borderTop: '1px solid',
});

const Component = ({ children }) => {
return <div css={styles}>{children}</div>;
};
```

Note that if you are using style composition (applying several `css` calls or `cssMap` objects to a component), the ordering of CSS properties is applied across the `css` and `cssMap` function call.

For example, the following code will cause an ESLint violation. This is because in `Component`, `paddingTop` is being applied before `padding`, which is the incorrect order.

```tsx
import { css, cssMap } from '@compiled/react';

const styles = cssMap({
root: {
paddingTop: '5px',
},
warning: {
// ...
},
});

const extraPadding = css({
padding: '5px',
});

const Component = ({ children }) => {
return <div css={[styles.root, extraPadding]}>{children}</div>;
};
```

## How do I fix this?

### \[Recommended\] Expand the shorthand property

Whenever possible, you should **expand the shorthand property**. For example:

```tsx
const styles = css({
paddingTop: token('space.050'),
padding: token('space.100'),
});
```

can be re-written as

```tsx
const styles = css({
paddingTop: token('space.050'),
paddingLeft: token('space.100'),
paddingRight: token('space.100'),
paddingBottom: token('space.100'),
});
```

If you're not sure what to expand the shorthand property to, you can refer to [resources like MDN](https://developer.mozilla.org/en-US/). For example, for `padding`, we see that [`padding` can be expanded to `paddingTop`, `paddingLeft`, `paddingRight`, and `paddingBottom`](https://developer.mozilla.org/en-US/docs/Web/CSS/padding).

### Re-order the properties

Sometimes this is not possible (e.g. if you are using an Atlassian Design System `font` token). If this is the case, you should re-order the shorthand and non-shorthand properties so that they are in the right order. The ESLint error message will tell you which properties are in the wrong order. For example:

```tsx
import { css } from '@compiled/react';
const styles = css({
lineHeight: '...',
font: '...',
fontWeight: '',
});
export const EmphasisText = ({ children }) => <span css={styles}>{children}</span>;
```

can be changed to

```tsx
import { css } from '@compiled/react';
const styles = css({
font: '...',
fontWeight: '',
lineHeight: '...',
});
export const EmphasisText = ({ children }) => <span css={styles}>{children}</span>;
```

## Examples

👎 Examples of **incorrect** code for this rule:

Expand All @@ -29,3 +144,8 @@ const styles = css({
borderColor: 'red',
});
```

## Found a bug?

- Atlassian employees: please contact us through the `#help-compiled` channel on Slack.
- Non-Atlassian employees should contact us [through the Issues page](https://github.com/atlassian-labs/compiled/issues).
Loading

0 comments on commit 88bbe38

Please sign in to comment.