Skip to content

Commit b9bc2e2

Browse files
zaerlclaude
andcommitted
Read color() and device-cmyk() as colours, and pin the guards the tests had missed
Second review pass. The list of functions that take a colour's channels was short of two. Three guards could be removed with the tests still passing: the dash after a hex run, the dash before a function's name, and a type check in the style rule that decided nothing, which is gone. Two comments said the wrong case pinned the string-only check. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 parent 022aebf commit b9bc2e2

4 files changed

Lines changed: 26 additions & 14 deletions

File tree

‎TESTING.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ Every matrix uses `fail-fast: false`, so a Windows failure never hides the macOS
224224
- End-to-end selectors read the text and roles already on screen. No `data-testid` — the visible copy is the contract, and renaming a button is a change worth noticing.
225225
- Journeys run in the `en-US` locale, the source language, so no translation catalog loads and they read English text as written. The exception is `tests/e2e/journeys/i18n.spec.js`, which launches with `lang: 'en-XA'` (Chromium's pseudo-locale) and fails on any visible text that did not go through `__()`. See [Translatable strings](CONTRIBUTING.md#translatable-strings).
226226
- What is on screen is named once, in `tests/e2e/helpers/ui.cjs`, for the journeys, the other end-to-end specs and `scripts/screenshots/` alike, so renaming a button or rebuilding a card is one change. It holds every control that more than one of those files reaches, and the locators that find a part of the screen rather than something to press: the rendered app, an entry of the sites list, what a window with no site says, the open site's heading, a card, the work-item card with its heading and its ticket's number, a ticket's row, the question asked before a deletion, and the window's confirmations. A sentence one journey asserts, a button only one journey presses, and a selector only one file needs stay in that file: they are what it is about.
227-
- No test reads a component's source, the `.jsx` under `src/renderer/`, as text. A test that greps the component for a call, a literal or the order of two strings pins the file rather than the screen: it goes red when the code moves and stays green when the screen is wrong. What is on the page and where, a journey asserts, with order read as document order rather than from bounding boxes (the page scrolls as cards appear). How a component is allowed to be written, a lint rule asserts: `react-hooks/refs` is on for the renderer for that reason, and so are this repository's own two, `toolkit/no-hardcoded-colors` and `toolkit/no-inline-styles`, now that the window is painted from classes in `src/renderer/shell.css` with the design system's tokens. The first reports a hex colour or a colour function written in a string, and the second a `style` attribute on a JSX element. Neither sees a colour's name, or a style that reaches an element some other way; each rule's header says what it does not read. The repository's rules are in `eslint-rules/`, and `tests/unit/eslint-rules.test.cjs` runs each over code that must pass and code that must not, since a rule that stopped reporting would turn nothing else red.
227+
- No test reads a component's source, the `.jsx` under `src/renderer/`, as text. A test that greps the component for a call, a literal or the order of two strings pins the file rather than the screen: it goes red when the code moves and stays green when the screen is wrong. What is on the page and where, a journey asserts, with order read as document order rather than from bounding boxes (the page scrolls as cards appear). How a component is allowed to be written, a lint rule asserts: `react-hooks/refs` is on for the renderer for that reason, and so are this repository's own two, `toolkit/no-hardcoded-colors` and `toolkit/no-inline-styles`, now that the window is painted from classes in `src/renderer/shell.css` with the design system's tokens. The first reports a hex colour or a function that takes a colour's channels, `rgb()` and the like, written in a string, and the second a `style` attribute on a JSX element. Neither sees a colour's name, or a style that reaches an element some other way; each rule's header says what it does not read. The repository's rules are in `eslint-rules/`, and `tests/unit/eslint-rules.test.cjs` runs each over code that must pass and code that must not, since a rule that stopped reporting would turn nothing else red.
228228
- Journeys mark each assertion as an **invariant** (must hold under any model of how work is stored) or a **characterisation** (true because of how the app stores things today). A red invariant is a bug; a red characterisation is a prompt to read it and update it deliberately.
229229
- A journey that needs something only a running process would say, a mail caught or a server's address, says it itself: `webContents.send` from `app.evaluate`, on the channel and in the shape the real sender uses. Check the payload against both ends, the handler in `src/preload.js` and the place `src/main.js` sends it, and say in the test what the stand-in leaves out, such as the store write the real sender makes first. To assert that the page did *not* react, wait until it has heard: after the telling, ask the main process one more question from the page (`window.api.getSitesWithMeta()` is a plain read) and assert once that answer is back. An absence read before then passes whatever the app does.
230230
- A bugfix's test must fail on the old code. A test written after the fix pins whatever the current behaviour is, rather than the correction.

‎eslint-rules/no-hardcoded-colors.cjs‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,20 +16,21 @@
1616
//
1717
// A rule of its own and not a selector given to `no-restricted-syntax`, which
1818
// could find the same nodes: the message names what was found, and the two
19-
// patterns are tested where they are written.
19+
// patterns are tested by themselves, through `hardcodedColor`.
2020

2121
// `#` and then 3, 4, 6 or 8 hex digits, and no more of a word on either side.
2222
// The lengths are CSS's own, so a Trac ticket's five digits are not a colour.
2323
// A reference of three or four digits written out in a string is one, as far
2424
// as this can tell: `#607` is a grey. Those are put together from the number
25-
// (`#%d`), which is how a translated string has to be built anyway.
25+
// (`#%d` in a translated string, `#${ n }` in a template), which is how the
26+
// renderer writes them already.
2627
// A `#` that follows a word, an `&` or a `/` is a fragment or an entity.
2728
const HEX_COLOR = /(?<![\w&/])#(?:[0-9a-f]{8}|[0-9a-f]{6}|[0-9a-f]{3,4})(?![\w-])/i;
2829

2930
// The functions that take a colour's channels, as part of no longer word.
3031
// `color-mix()` is not among them: what it mixes is given to it, and can be
3132
// tokens.
32-
const COLOR_FUNCTION = /(?<![\w-])(?:rgba?|hsla?|hwb|lab|lch|oklab|oklch)\(/i;
33+
const COLOR_FUNCTION = /(?<![\w-])(?:rgba?|hsla?|hwb|lab|lch|oklab|oklch|color|device-cmyk)\(/i;
3334

3435
/**
3536
* The colour a string spells out, if it spells one.
@@ -59,14 +60,15 @@ module.exports = /** @type {import('eslint').Rule.RuleModule} */ ( {
5960
},
6061
create( context ) {
6162
const check = ( node, text ) => {
62-
// A number, a pattern, and a template's text that cannot be read as
63-
// a string are not strings.
63+
// A number and a pattern are literals too, and are not strings.
6464
if ( typeof text !== 'string' ) return;
6565
const color = hardcodedColor( text );
6666
if ( color ) context.report( { node, messageId: 'hardcoded', data: { color } } );
6767
};
6868
return {
6969
Literal: ( node ) => check( node, node.value ),
70+
// The text as the program gets it. Where an escape cannot be read
71+
// there is none, and nothing to look in.
7072
TemplateElement: ( node ) => check( node, node.value.cooked ),
7173
};
7274
},

‎eslint-rules/no-inline-styles.cjs‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,12 @@
1313
// It reads the attribute as written on a JSX element, and nothing else: not a
1414
// `style` handed over in a spread, and not one set on an element from an
1515
// effect. A selector given to `no-restricted-syntax` could find the same
16-
// attribute; this is a rule of its own so that it is tested beside the other
17-
// one, and says why in its own words. A value that can only
18-
// be known while the app runs, a width measured from something, has no class
19-
// to be; that one is disabled where it stands, with the reason beside it.
16+
// attribute and say the same thing; this is a rule of its own so that it is
17+
// tested beside the other one.
18+
//
19+
// A value that can only be known while the app runs, a width measured from
20+
// something, has no class to be; that one is disabled where it stands, with
21+
// the reason beside it.
2022

2123
module.exports = /** @type {import('eslint').Rule.RuleModule} */ ( {
2224
meta: {
@@ -33,7 +35,9 @@ module.exports = /** @type {import('eslint').Rule.RuleModule} */ ( {
3335
create( context ) {
3436
return {
3537
JSXAttribute( node ) {
36-
if ( node.name.type === 'JSXIdentifier' && node.name.name === 'style' ) {
38+
// An attribute in a namespace has a name made of two; its `name`
39+
// is not this string.
40+
if ( node.name.name === 'style' ) {
3741
context.report( { node, messageId: 'inline' } );
3842
}
3943
},

‎tests/unit/eslint-rules.test.cjs‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,13 +40,17 @@ tester.run( 'no-hardcoded-colors', rules[ 'no-hardcoded-colors' ], {
4040
// A fragment and an entity are not colours, whatever their letters.
4141
"const link = 'https://example.org/page#fade';",
4242
"const anchor = 'https://example.org/handbook/#add';",
43+
"const section = '#bad-request';",
4344
"const entity = '&#128;';",
44-
// A word that only ends like a function.
45+
// A word that only ends like a function, and a name that goes on.
4546
"const label = 'Collab(tm)';",
47+
"const own = 'my-rgb(1, 2, 3)';",
48+
// A function's name is one only in front of its bracket.
4649
"const mixed = 'color-mix(in oklab, var(--wpds-color-stroke-surface-warning), transparent)';",
47-
// The text of a template that cannot be read as a string is not one.
50+
// A template is read as the program gets it, and where an escape
51+
// cannot be read the program gets nothing.
4852
'const raw = String.raw`\\unicode #fff`;',
49-
// A number is not a string, a comment is not code, and a pattern that
53+
// A comment is not code, and only a string is read: a pattern that
5054
// looks for a colour does not paint with one.
5155
'const count = 0xfff; // #fff',
5256
'const looksForOne = / #fff$/;',
@@ -64,6 +68,8 @@ tester.run( 'no-hardcoded-colors', rules[ 'no-hardcoded-colors' ], {
6468
{ code: "const note = 'Fixed in #607';", errors: [ { messageId: 'hardcoded', data: { color: '#607' } } ] },
6569
{ code: "const wash = 'rgba(46,160,67,0.18)';", errors: [ { messageId: 'hardcoded', data: { color: 'rgba(…)' } } ] },
6670
{ code: "const tint = 'HSL(210 50% 40%)';", errors: [ { messageId: 'hardcoded', data: { color: 'HSL(…)' } } ] },
71+
{ code: "const wide = 'color(display-p3 1 0.5 0)';", errors: [ { messageId: 'hardcoded', data: { color: 'color(…)' } } ] },
72+
{ code: "const ink = 'device-cmyk(0 0 0 1)';", errors: [ { messageId: 'hardcoded', data: { color: 'device-cmyk(…)' } } ] },
6773
{ code: 'const edge = `2px solid #3858e9`;', errors: [ { messageId: 'hardcoded' } ] },
6874
{ code: 'const edge = `${ width }px solid oklch(60% 0.2 250)`;', errors: [ { messageId: 'hardcoded' } ] },
6975
{ code: 'const node = <p className="x" title="#fff">text</p>;', errors: [ { messageId: 'hardcoded' } ] },

0 commit comments

Comments
 (0)