-
Notifications
You must be signed in to change notification settings - Fork 4k
[Typography] Add semantic type tokens and block new raw font sizes #96880
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.
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
Changes from 5 commits
28099bb
ae34e65
cfa5bb1
b8ff6e7
9bbefa5
b6fa548
6db2269
3f17671
2c9c878
1ac4e0c
50b8207
543eb6d
04da195
ef521fc
e7cdf9f
5b4fdc6
41fb8bb
c1ed760
ae61851
a98e180
44cae82
4474d96
6140382
c0e60b2
bf8e619
bc07863
8c538fc
566d0d8
af1b538
90889e8
c226807
61dbba6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| const name = 'no-raw-typography'; | ||
|
|
||
| const meta = { | ||
| type: 'problem', | ||
| docs: { | ||
| description: 'Disallow raw numeric fontSize/lineHeight values. Type must come from the typography scale so it cannot drift from the design system.', | ||
| recommended: 'error', | ||
| }, | ||
| schema: [], | ||
| messages: { | ||
| rawTypography: 'Raw `{{property}}: {{value}}` is not allowed. Use a `<Text variant="...">` or a token from src/styles/typography.ts (https://github.com/Expensify/App/issues/37503).', | ||
| }, | ||
| }; | ||
|
|
||
| const BANNED_PROPERTIES = new Set(['fontSize', 'lineHeight']); | ||
|
|
||
| /** | ||
| * @param {import('estree').Node} key | ||
| * @returns {string | undefined} | ||
| */ | ||
| function getPropertyName(key) { | ||
| if (key.type === 'Identifier') { | ||
| return key.name; | ||
| } | ||
| if (key.type === 'Literal' && typeof key.value === 'string') { | ||
| return key.value; | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| /** | ||
| * @param {import('estree').Node} node | ||
| * @returns {boolean} | ||
| */ | ||
| function isNumericLiteral(node) { | ||
| if (node.type === 'Literal' && typeof node.value === 'number') { | ||
| return true; | ||
| } | ||
| return node.type === 'UnaryExpression' && node.operator === '-' && isNumericLiteral(node.argument); | ||
|
grgia marked this conversation as resolved.
Outdated
grgia marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| /** | ||
| * Flags object properties (`{fontSize: 17}`) and JSX attributes (`<Text fontSize={17}>`) that set | ||
| * `fontSize`/`lineHeight` to a numeric literal. References to tokens or computed values are allowed. | ||
| * | ||
| * @param {import('eslint').Rule.RuleContext} context | ||
| * @returns {import('eslint').Rule.RuleListener} | ||
| */ | ||
| function create(context) { | ||
| function report(valueNode, propertyName) { | ||
| context.report({ | ||
| node: valueNode, | ||
| messageId: 'rawTypography', | ||
| data: { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a style is built or mutated with an assignment such as Useful? React with 👍 / 👎. |
||
| property: propertyName, | ||
| value: context.sourceCode.getText(valueNode), | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| return { | ||
| Property(node) { | ||
| if (node.computed) { | ||
| return; | ||
| } | ||
| const propertyName = getPropertyName(node.key); | ||
| if (propertyName === undefined || !BANNED_PROPERTIES.has(propertyName) || !isNumericLiteral(node.value)) { | ||
| return; | ||
| } | ||
| report(node.value, propertyName); | ||
| }, | ||
| JSXAttribute(node) { | ||
| if (node.name.type !== 'JSXIdentifier' || !BANNED_PROPERTIES.has(node.name.name)) { | ||
| return; | ||
| } | ||
| if (node.value?.type !== 'JSXExpressionContainer' || !isNumericLiteral(node.value.expression)) { | ||
| return; | ||
| } | ||
| report(node.value.expression, node.name.name); | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| export {name, meta, create}; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,8 +96,7 @@ function QRShare({ | |
|
|
||
| {!!title && ( | ||
| <Text | ||
| family="EXP_NEW_KANSAS_MEDIUM" | ||
| fontSize={variables.fontSizeXLarge} | ||
| variant="h1" | ||
| numberOfLines={2} | ||
| style={styles.qrShareTitle} | ||
| > | ||
|
|
@@ -107,7 +106,7 @@ function QRShare({ | |
|
|
||
| {!!subtitle && ( | ||
| <Text | ||
| fontSize={variables.fontSizeLabel} | ||
| variant="label" | ||
|
Comment on lines
108
to
+109
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Example |
||
| numberOfLines={2} | ||
| style={[styles.mt1, styles.textAlignCenter]} | ||
| color={theme.textSupporting} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all changes in this file necessary? We should not add more eslint disablings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each new row is an existing violation of the two checks this PR introduces: raw fontSize/lineHeight values, and the now-deprecated fontSize prop on Text. Listing them at today's counts lets both checks ship as errors without migrating all ~60 call sites in this PR. They get removed as files migrate to the tokens.