Repository navigation
feat: Include typography tokens to accept density values - #216
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #216 +/- ##
==========================================
+ Coverage 97.68% 97.69% +0.01%
==========================================
Files 54 54
Lines 1469 1478 +9
Branches 273 274 +1
==========================================
+ Hits 1435 1444 +9
Misses 29 29
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The schema changes are backward-compatible and thoroughly tested across supported typography categories.
Review effort: Balanced
Findings: None
What changed in this PR
Extends theme JSON validation so typography tokens support both scalar strings and density-specific values.
Changes:
- Adds density-aware schemas for typography tokens.
- Improves
anyOfvalidation diagnostics. - Adds comprehensive validation tests.
| File | Description |
|---|---|
src/build/tasks/theme-json-schema.ts |
Adds density schemas and expanded errors. |
src/build/tasks/__tests__/theme-json-schema.test.ts |
Tests valid and invalid density typography values. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| patternProperties: { | ||
| '^color-': getTokenSchema(getComplexValueSchema(colorValueSchema, visualModes)), | ||
| '^font-family-': getTokenSchema(stringValueSchema), | ||
| '^font-family-': getTokenSchema({ |
There was a problem hiding this comment.
It's feels strange to see that the font-family changes between, density (comfortable, and compact), Is there a use case for density scoped font-family?
There was a problem hiding this comment.
there's no known use-case, but we thought it better to apply this consistently to all font-related properties for consistency
There was a problem hiding this comment.
Fair point on consistency. But there's no use case and this just adds unused schema surface for font-family. Widening it from string to string | density object later is backward-compatible, so we can add it whenever a real need comes up. No strong opinion though, if you'd rather keep all font-* uniform, I won't block on it.
There was a problem hiding this comment.
Fair point, I think it makes sense to remove it for now. Updated.
|
|
||
| // Returns the full thrown message so assertions can check the complete | ||
| // expanded string, not just a prefix. | ||
| const getError = (tokens: Record<string, TokenJson>): string => { |
There was a problem hiding this comment.
Instead of using getError method. Can we make sure the tests use similar behavior as the others, that would make it easier to read, and understand the test file.
something like this?
expect(() =>
validateTokens(...),
).toThrowError(
'Tokens validation error: instance.tokens.font-size-body.$value is not any of ' +
'(subschema 0: instance does not match pattern "^\\d+(\\.\\d+)?(px|rem|em)$" | ' +
'subschema 1: instance is not of a type(s) object)',
);
| }), | ||
| ).toThrowError( | ||
| 'Tokens validation error: instance.tokens.letter-spacing-button.$value does not match pattern "^(normal|inherit|initial|revert|revert-layer|unset|-?\\\\d*\\\\.?\\\\d+(px|rem|em))$"', | ||
| 'Tokens validation error: instance.tokens.letter-spacing-button.$value is not any of (subschema 0:', |
There was a problem hiding this comment.
The new seems wrong, and also it removed the pattern it matched with.
| const message = getError({ | ||
| 'font-size-body-m': { $value: { comfortable: 'big', compact: 'small' } }, | ||
| }); | ||
| // both offending modes are reported, separated by '; ' |
There was a problem hiding this comment.
Nit: for all the comments can we consider removing them? it seems like the test description, and the thrown errors give enough description.
| ).toThrowError( | ||
| 'Tokens validation error: instance.tokens.font-size-body.$value does not match pattern "^\\\\d+(\\\\.\\\\d+)?(px|rem|em)$"', | ||
| ); | ||
| ).toThrowError('Tokens validation error: instance.tokens.font-size-body.$value is not any of (subschema 0:'); |
There was a problem hiding this comment.
Nit: can we also update this test to use something like this since it's matching only the initial substring right now?
.toThrowError(
'Tokens validation error: instance.tokens.font-size-body.$value is not any of ' +
'(subschema 0: instance does not match pattern "^\\\\d+(\\\\.\\\\d+)?(px|rem|em)$" | ' +
'subschema 1: instance is not of a type(s) object)',
);
| ).toThrowError( | ||
| 'Tokens validation error: instance.tokens.line-height-body.$value does not match pattern "^\\\\d+(\\\\.\\\\d+)?(px|rem|em)$"', | ||
| ); | ||
| ).toThrowError('Tokens validation error: instance.tokens.line-height-body.$value is not any of (subschema 0:'); |
There was a problem hiding this comment.
Same as the above, update to have the full error instead of being cut after (subschema 0:
Issue #, if available:
As part of density project, compact font-size needs to be reduced. Currently typography tokens can only accept string and this PR makes it possible to pass either strings or density values.
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.