diff --git a/__tests__/typography.js b/__tests__/typography.js index 2872cde5..3e6d5a47 100644 --- a/__tests__/typography.js +++ b/__tests__/typography.js @@ -1,67 +1,222 @@ -import dedent from 'dedent' -import stylelint from 'stylelint' -import pluginPath from '../plugins/typography.js' +// TODO: add tests for hard-coded values that match multiple variables +import plugin from '../plugins/typography.js' -const ruleName = 'primer/typography' -const configWithOptions = args => ({ - plugins: [pluginPath], - rules: { - [ruleName]: args, - }, -}) - -describe(ruleName, () => { - describe('font-size', () => { - it('reports properties w/o variables', () => { - return stylelint - .lint({ - code: dedent` - .x { font-size: 11px; } - .y { font-size: $spacer-3; } - `, - config: configWithOptions(true), - }) - .then(data => { - expect(data).toHaveErrored() - expect(data).toHaveWarnings([ - `Please use a font-size variable instead of "11px". See https://primer.style/css/utilities/typography. (${ruleName})`, - `Please use a font-size variable instead of "$spacer-3". See https://primer.style/css/utilities/typography. (${ruleName})`, - ]) - }) - }) - - it('does not report font size variables', () => { - return stylelint - .lint({ - code: dedent` - .h1 { font-size: $h1-size; } - .h2 { font-size: $h2-size; } - small { font-size: $font-size-small; } - `, - config: configWithOptions(true), - }) - .then(data => { - expect(data).not.toHaveErrored() - expect(data).toHaveWarningsLength(0) - }) - }) +const plugins = [plugin] +const { + ruleName, + rule: {messages}, +} = plugin - it('can fix them', () => { - return stylelint - .lint({ - code: dedent` - .x { font-size: 32px; } - `, - config: configWithOptions(true, {verbose: true}), - fix: true, - }) - .then(data => { - expect(data).not.toHaveErrored() - expect(data).toHaveWarningsLength(0) - expect(data.output).toEqual(dedent` - .x { font-size: $h1-size; } - `) - }) - }) - }) +// General Tests +testRule({ + plugins, + ruleName, + config: [true, {}], + fix: true, + cache: false, + accept: [ + // Font sizes + { + code: '.x { font-size: var(--text-title-size-medium); }', + description: 'CSS > Accepts font size variables', + }, + // Font weights + { + code: '.x { font-weight: var(--base-text-weight-semibold); }', + description: 'CSS > Accepts base font weight variables', + }, + { + code: '.x { font-weight: var(--text-title-weight-medium); }', + description: 'CSS > Accepts functional font weight variables', + }, + // Line heights + { + code: '.x { line-height: var(--text-title-lineHeight-medium); }', + description: 'CSS > Accepts line height variables', + }, + // Font family + { + code: '.x { font-family: var(--fontStack-system); }', + description: 'CSS > Accepts font stack variables', + }, + // Font shorthand + { + code: '.x { font: var(--text-display-shorthand); }', + description: 'CSS > Accepts font shorthand variables', + }, + ], + reject: [ + // Font sizes + { + code: '.x { font-size: 42px; }', + unfixable: true, + message: messages.rejected('42px'), + line: 1, + column: 17, + endColumn: 21, + description: 'CSS > Errors on value not in font size list', + }, + { + code: '.x { font-size: 40px; }', + fixed: '.x { font-size: var(--text-display-size); }', + message: messages.rejected('40px', {name: '--text-display-size'}), + line: 1, + column: 17, + endColumn: 21, + description: "CSS > Replaces '40px' with 'var(--text-display-size)'.", + }, + { + code: '.x { font-size: 2.5rem; }', + fixed: '.x { font-size: var(--text-display-size); }', + message: messages.rejected('2.5rem', {name: '--text-display-size'}), + line: 1, + column: 17, + endColumn: 23, + description: "CSS > Replaces '2.5rem' with 'var(--text-display-size)'.", + }, + { + code: '.x { font-size: 1.25rem; }', + unfixable: true, + message: messages.rejected('1.25rem', [{ name: '--text-title-size-medium' }, { name: '--text-subtitle-size' }]), + line: 1, + column: 17, + endColumn: 24, + description: "CSS > Suggests list of variables to replace '1.25rem' with.", + }, + // Font weights + { + code: '.x { font-weight: 500; }', + unfixable: true, + message: messages.rejected('500', [{name: '--base-text-weight-medium'}, {name: '--text-display-weight'}]), + line: 1, + column: 19, + endColumn: 22, + description: "CSS > Errors on font-weight of 500 and suggests '--base-text-weight-medium' or '--text-display-weight'.", + }, + { + code: '.x { font-weight: 100; }', + fixed: '.x { font-weight: var(--base-text-weight-light); }', + message: messages.rejected('100', {name: '--base-text-weight-light'}), + line: 1, + column: 19, + endColumn: 22, + description: "CSS > Replaces font-weight less than 300 with 'var(--base-text-weight-light)'.", + }, + { + code: '.x { font-weight: 800; }', + unfixable: true, + message: messages.rejected('800', [{name: '--base-text-weight-semibold'}, {name: '--text-title-weight-large'}, {name: '--text-title-weight-medium'}, {name: '--text-title-weight-small'}]), + line: 1, + column: 19, + endColumn: 22, + description: "CSS > Errors on font-weight greater than 600 and suggests '--base-text-weight-semibold', '--text-title-weight-large', '--text-title-weight-medium', or '--text-title-weight-small'.", + }, + { + code: '.x { font-weight: lighter; }', + fixed: '.x { font-weight: var(--base-text-weight-light); }', + message: messages.rejected('lighter', {name: '--base-text-weight-light'}), + line: 1, + column: 19, + endColumn: 26, + description: "CSS > Replaces 'lighter' font-weight keyword with 'var(--base-text-weight-light)'.", + }, + { + code: '.x { font-weight: bold; }', + unfixable: true, + message: messages.rejected('bold', [{name: '--base-text-weight-semibold'}, {name: '--text-title-weight-large'}, {name: '--text-title-weight-medium'}, {name: '--text-title-weight-small'}]), + line: 1, + column: 19, + endColumn: 23, + description: "CSS > Errors on 'bold' font-weight keyword and suggests '--base-text-weight-semibold', '--text-title-weight-large', '--text-title-weight-medium', or '--text-title-weight-small'.", + }, + { + code: '.x { font-weight: bolder; }', + unfixable: true, + message: messages.rejected('bolder', [{name: '--base-text-weight-semibold'}, {name: '--text-title-weight-large'}, {name: '--text-title-weight-medium'}, {name: '--text-title-weight-small'}]), + line: 1, + column: 19, + endColumn: 25, + description: "CSS > Errors on 'bolder' font-weight keyword and suggests '--base-text-weight-semibold', '--text-title-weight-large', '--text-title-weight-medium', or '--text-title-weight-small'.", + }, + { + code: '.x { font-weight: normal; }', + unfixable: true, + message: messages.rejected('normal', [{name: '--base-text-weight-normal'}, {name: '--text-subtitle-weight'}, {name: '--text-body-weight'}, {name: '--text-caption-weight'}, {name: '--text-codeBlock-weight'}, {name: '--text-codeInline-weight'}]), + line: 1, + column: 19, + endColumn: 25, + description: "CSS > Errors on 'normal' font-weight keyword and suggests '--base-text-weight-normal', '--text-subtitle-weight', '--text-body-weight', '--text-caption-weight', '--text-codeBlock-weight' or '--text-codeInline-weight'.", + }, + // Line heights + { + code: '.x { line-height: 42px; }', + unfixable: true, + message: messages.rejected('42px'), + line: 1, + column: 19, + endColumn: 23, + description: 'CSS > Errors on value not in line height list', + }, + { + code: '.x { line-height: 1.4; }', + fixed: '.x { line-height: var(--text-display-lineHeight); }', + message: messages.rejected('1.4', {name: '--text-display-lineHeight'}), + line: 1, + column: 19, + endColumn: 22, + description: "CSS > Replaces '1.4' line-height with 'var(--text-display-lineHeight)'.", + }, + { + code: '.x { line-height: 1.5; }', + unfixable: true, + message: messages.rejected('1.5', [{name: '--text-title-lineHeight-large'}, {name: '--text-title-lineHeight-small'}, {name: '--text-body-lineHeight-large'}]), + line: 1, + column: 19, + endColumn: 22, + description: "CSS > Errors on '1.5' line-height and suggests '--text-title-lineHeight-large', '--text-title-lineHeight-small', or '--text-body-lineHeight-large'.", + }, + // Font family + // TODO: figure out how to handle values with spaces in them + // Will these be unfixable since we won't know for sure that they want `--fontStack-system`/`--fontStack-sansSerif` and not `--fontStack-monospace`? + { + code: '.x { font-family: Comic Sans; }', + unfixable: true, + message: messages.rejected('Comic Sans'), + line: 1, + column: 14, + endColumn: 18, + description: 'CSS > Errors on value not in font family list', + }, + // Font shorthand + { + code: '.x { font: bold 24px/1 sans-serif; }', + unfixable: true, + message: messages.rejected('small-caps bold 24px/1 sans-serif'), + line: 1, + column: 14, + endColumn: 18, + description: 'CSS > Errors on hard-coded value not in font shorthand list', + }, + // // should we allow this to pass since the value is the same as `--text-display-shorthand`? + // // should we suggest `--text-display-shorthand` as a replacement? + // { + // code: '.x { font: var(--text-display-weight) var(--text-display-size) / var(--text-display-lineHeight) var(--fontStack-sansSerif); }', + // unfixable: true, + // message: messages.rejected('var(--text-display-weight) var(--text-display-size) / var(--text-display-lineHeight) var(--fontStack-sansSerif)'), + // line: 1, + // column: 14, + // endColumn: 18, + // description: 'CSS > Errors on CSS variables not in font shorthand list', + // }, + // // only test for this if we want to allow people to use `font` shorthand with valid CSS variable combinations + // // { + // // code: '.x { font: var(--base-text-weight-normal) var(--text-display-size) / var(--text-body-lineHeight-small) var(--fontStack-sansSerif); }', + // // unfixable: true, + // // message: messages.rejected('var(--base-text-weight-normal) var(--text-display-size) / var(--text-body-lineHeight-small) var(--fontStack-sansSerif)'), + // // line: 1, + // // column: 14, + // // endColumn: 18, + // // description: 'CSS > Errors on CSS variable combinations that do not match font shorthand variable values', + // // }, + ], }) diff --git a/plugins/lib/primitives.js b/plugins/lib/primitives.js deleted file mode 100644 index bb8d42aa..00000000 --- a/plugins/lib/primitives.js +++ /dev/null @@ -1,33 +0,0 @@ -import {createRequire} from 'node:module' - -const require = createRequire(import.meta.url) - -export async function primitivesVariables(type) { - const variables = [] - - const files = [] - switch (type) { - case 'size': - files.push('base/size/size.json') - break - } - - for (const file of files) { - // eslint-disable-next-line import/no-dynamic-require - const data = require(`@primer/primitives/dist/styleLint/${file}`) - - for (const key of Object.keys(data)) { - const size = data[key] - const values = size['value'] - values.push(`${parseInt(size['original']['value']) + 1}px`) - values.push(`${parseInt(size['original']['value']) - 1}px`) - - variables.push({ - name: `--${size['name']}`, - values, - }) - } - } - - return variables -} diff --git a/plugins/lib/utils.js b/plugins/lib/utils.js new file mode 100644 index 00000000..ddc6c868 --- /dev/null +++ b/plugins/lib/utils.js @@ -0,0 +1,49 @@ +import {createRequire} from 'node:module' + +const require = createRequire(import.meta.url) + +export function primitivesVariables(type) { + const variables = [] + + const files = [] + switch (type) { + case 'spacing': + files.push('base/size/size.json') + break + case 'border': + files.push('functional/size/border.json') + break + case 'typography': + files.push('base/typography/typography.json') + files.push('functional/typography/typography.json') + break +} + + for (const file of files) { + // eslint-disable-next-line import/no-dynamic-require + const data = require(`@primer/primitives/dist/styleLint/${file}`) + + for (const key of Object.keys(data)) { + const size = data[key] + const values = typeof size['value'] === 'string' ? [size['value']] : size['value'] + + variables.push({ + name: `--${size['name']}`, + values, + }) + } + } + + return variables +} + +export function walkGroups(root, validate) { + for (const node of root.nodes) { + if (node.type === 'function') { + walkGroups(node, validate) + } else { + validate(node) + } + } + return root +} \ No newline at end of file diff --git a/plugins/typography.js b/plugins/typography.js index ad1ee0c9..6a237cef 100644 --- a/plugins/typography.js +++ b/plugins/typography.js @@ -1,24 +1,207 @@ -import {createVariableRule} from './lib/variable-rules.js' - -export default createVariableRule( - 'primer/typography', - { - 'font-size': { - expects: 'a font-size variable', - values: ['$body-font-size', '$h{000,00,0,1,2,3,4,5,6}-size', '$font-size-*', '1', '1em', 'inherit'], - }, - 'font-weight': { - props: 'font-weight', - values: ['$font-weight-*', 'inherit'], - replacements: { - bold: '$font-weight-bold', - normal: '$font-weight-normal', - }, - }, - 'line-height': { - props: 'line-height', - values: ['$body-line-height', '$lh-*', '0', '1', '1em', 'inherit'], - }, +import stylelint from 'stylelint' +import declarationValueIndex from 'stylelint/lib/utils/declarationValueIndex.cjs' +import valueParser from 'postcss-value-parser' +import {primitivesVariables, walkGroups} from './lib/utils.js' + +const { + createPlugin, + utils: {report, ruleMessages, validateOptions}, +} = stylelint + +export const ruleName = 'primer/typography' +export const messages = ruleMessages(ruleName, { + rejected: (value, replacement, propName) => { + // no possible replacement + if (!replacement) { + return `Please use a Primer typography variable instead of '${value}'. Consult the primer docs for a suitable replacement. https://primer.style/foundations/primitives/typography` + } + + // multiple possible replacements + if (replacement.length) { + return `Please use one of the following Primer typography variables instead of '${value}': ${replacement.map(replacementObj => `'${replacementObj.name}'`).join(', ')}. https://primer.style/foundations/primitives/typography` + } + + // one possible replacement + return `Please replace '${value}' with Primer typography variable '${replacement['name']}'. https://primer.style/foundations/primitives/typography` }, - 'https://primer.style/css/utilities/typography', -) +}) + +const fontWeightKeywordMap = { + normal: 400, + bold: 600, + bolder: 600, + lighter: 300, +} +const getClosestFontWeight = (goalWeightNumber, fontWeightsTokens) => { + return fontWeightsTokens.reduce((prev, curr) => + Math.abs(curr.values - goalWeightNumber) < Math.abs(prev.values - goalWeightNumber) ? curr : prev + ).values +} + +const variables = primitivesVariables('typography') +const fontSizes = [] +const fontWeights = [] +const lineHeights = [] +const fontStacks = [] +const fontShorthands = [] + +// Props that we want to check for typography variables +const propList = ['font-size', 'font-weight', 'line-height', 'font-family', 'font'] + +for (const variable of variables) { + const name = variable['name'] + + if (name.includes('size')) { + fontSizes.push(variable) + } + + if (name.includes('weight')) { + fontWeights.push(variable) + } + + if (name.includes('lineHeight')) { + lineHeights.push(variable) + } + + if (name.includes('fontStack')) { + fontStacks.push(variable) + } + + if (name.includes('shorthand')) { + fontShorthands.push(variable) + } +} + +/** @type {import('stylelint').Rule} */ +const ruleFunction = (primary, secondaryOptions, context) => { + return (root, result) => { + const validOptions = validateOptions(result, ruleName, { + actual: primary, + possible: [true], + }) + let validValues = [] + + if (!validOptions) return + + root.walkDecls(declNode => { + const {prop, value} = declNode + + if (!propList.some(typographyProp => prop.startsWith(typographyProp))) return + + const problems = [] + + // Possible way to get around values with spaces: just do something else for those props + // This might help instead of `valueParser`: https://github.com/bramstein/css-font-parser + // const parsedValue = prop === 'font-family' ? : walkGroups(valueParser(value), node => { + const parsedValue = walkGroups(valueParser(value), node => { + const checkForVariable = (vars, nodeValue) => + vars.some(variable => + new RegExp(`${variable['name'].replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\b`).test(nodeValue), + ) + + // Only check word types. https://github.com/TrySound/postcss-value-parser#word + if (node.type !== 'word') { + return + } + + // Exact values to ignore. + if (node.value === 'inherit') { + return + } + + switch(prop) { + case 'font-size': + validValues = fontSizes + break + case 'font-weight': + validValues = fontWeights + break + case 'line-height': + validValues = lineHeights + break + case 'font-family': + validValues = fontStacks + break + case 'font': + validValues = fontShorthands + break + default: + validValues = [] + } + + + if (checkForVariable(validValues, node.value)) { + return + } + + // TODO: clean this up + const getReplacements = () => { + const replacementTokens = validValues.filter(variable => { + // if `variable.values` is a string or number like we'd get from `line-height: 1.6` or `font-weight: 500` + // we can just compare the values directly + if (!(variable.values instanceof Array)) { + let nodeValue = node.value + + if (prop === 'font-weight') { + nodeValue = getClosestFontWeight(fontWeightKeywordMap[node.value] || node.value, fontWeights) + } + + return variable.values.toString() === nodeValue.toString() + } + + return variable.values.includes(node.value.replace('-', '')) + }) + + if (!replacementTokens.length) { + return + } + + if (replacementTokens.length > 1) { + return replacementTokens + } + + return replacementTokens[0] + } + const replacement = getReplacements() + const fixable = replacement && !replacement.length + + if (fixable && context.fix) { + node.value = node.value.replace(node.value, `var(${replacement['name']})`) + } else { + problems.push({ + index: declarationValueIndex(declNode) + node.sourceIndex, + endIndex: declarationValueIndex(declNode) + node.sourceIndex + node.value.length, + message: messages.rejected(node.value, replacement, prop), + }) + } + + return + }) + + if (context.fix) { + declNode.value = parsedValue.toString() + } + + if (problems.length) { + for (const err of problems) { + report({ + index: err.index, + endIndex: err.endIndex, + message: err.message, + node: declNode, + result, + ruleName, + }) + } + } + }) + } +} + +ruleFunction.ruleName = ruleName +ruleFunction.messages = messages +ruleFunction.meta = { + fixable: true, +} + +export default createPlugin(ruleName, ruleFunction)