Skip to content
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

Fix column position and ignore Sass function for font-weight-notation #6005

Merged
merged 2 commits into from Apr 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 17 additions & 3 deletions lib/rules/font-weight-notation/README.md
Expand Up @@ -4,13 +4,17 @@ Require numeric or named (where possible) `font-weight` values. Also, when named

<!-- prettier-ignore -->
```css
a { font-weight: bold }
a { font-weight: bold; }
/** ↑
* This notation */
* This notation */

a { font: italic small-caps 600 16px/3 cursive; }
/** ↑
* And this notation, too */
* And this notation, too */

@font-face { font-weight: normal bold; }
/** ↑
* Multiple notations are available in @font-face */
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[note] I think adding a description about @font-face makes sense. Any thoughts?

See also https://developer.mozilla.org/en-US/docs/Web/CSS/@font-face/font-weight

```

Valid font-weight names are `normal`, `bold`, `bolder`, and `lighter`.
Expand All @@ -37,6 +41,11 @@ a { font-weight: bold; }
a { font: italic normal 20px sans-serif; }
```

<!-- prettier-ignore -->
```css
@font-face { font-weight: normal bold; }
```

The following patterns are _not_ considered problems:

<!-- prettier-ignore -->
Expand All @@ -49,6 +58,11 @@ a { font-weight: 700; }
a { font: italic 400 20px; }
```

<!-- prettier-ignore -->
```css
@font-face { font-weight: 400 700; }
```

### `"named-where-possible"`

`font-weight` values _must always_ be keywords when an appropriate keyword is available.
Expand Down
10 changes: 9 additions & 1 deletion lib/rules/font-weight-notation/__tests__/index.js
Expand Up @@ -180,7 +180,7 @@ testRule({
code: '@font-face { font-weight: 400 bold; }',
message: messages.expected('numeric'),
line: 1,
column: 27,
column: 31,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[note] The position is changed to bold, not 400.

},
{
code: '@font-face { font-weight: normal 700; }',
Expand Down Expand Up @@ -380,5 +380,13 @@ testRule({
code: 'a { font-weight: $foo }',
description: 'ignore sass variable',
},
{
code: 'a { font-weight: map-deep-get($theme, typography, weight, semibold); }',
description: 'ignore sass function',
},
{
code: 'a { font-weight: /*comment*/ map-deep-get($theme, typography, weight, semibold); }',
description: 'ignore sass function with comment',
},
],
});
106 changes: 73 additions & 33 deletions lib/rules/font-weight-notation/index.js
@@ -1,12 +1,13 @@
'use strict';

const valueParser = require('postcss-value-parser');

const declarationValueIndex = require('../../utils/declarationValueIndex');
const isNumbery = require('../../utils/isNumbery');
const isStandardSyntaxValue = require('../../utils/isStandardSyntaxValue');
const isVariable = require('../../utils/isVariable');
const keywordSets = require('../../reference/keywordSets');
const optionsMatches = require('../../utils/optionsMatches');
const postcss = require('postcss');
const report = require('../../utils/report');
const ruleMessages = require('../../utils/ruleMessages');
const validateOptions = require('../../utils/validateOptions');
Expand Down Expand Up @@ -51,12 +52,12 @@ const rule = (primary, secondaryOptions) => {
return;
}

root.walkDecls((decl) => {
if (decl.prop.toLowerCase() === 'font-weight') {
checkWeight(decl.value, decl);
}
root.walkDecls(/^font(-weight)?$/i, (decl) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

const prop = decl.prop.toLowerCase();

if (decl.prop.toLowerCase() === 'font') {
if (prop === 'font-weight') {
checkWeight(decl, decl.value);
} else if (prop === 'font') {
checkFont(decl);
}
});
Expand All @@ -65,31 +66,35 @@ const rule = (primary, secondaryOptions) => {
* @param {import('postcss').Declaration} decl
*/
function checkFont(decl) {
const valueList = postcss.list.space(decl.value);
const valueNodes = findFontWeights(decl.value);

// We do not need to more carefully distinguish font-weight
// numbers from unitless line-heights because line-heights in
// `font` values need to be part of a font-size/line-height pair
const hasNumericFontWeight = valueList.some((value) => isNumbery(value));
const hasNumericFontWeight = valueNodes.some(({ value }) => isNumbery(value));

for (const valueNode of valueNodes) {
const value = valueNode.value;
const lowerValue = value.toLowerCase();

for (const value of postcss.list.space(decl.value)) {
if (
(value.toLowerCase() === NORMAL_KEYWORD && !hasNumericFontWeight) ||
(lowerValue === NORMAL_KEYWORD && !hasNumericFontWeight) ||
isNumbery(value) ||
(value.toLowerCase() !== NORMAL_KEYWORD &&
keywordSets.fontWeightKeywords.has(value.toLowerCase()))
(lowerValue !== NORMAL_KEYWORD && keywordSets.fontWeightKeywords.has(lowerValue))
) {
checkWeight(value, decl);
checkWeight(decl, value, valueNode);

return;
}
}
}

/**
* @param {string} weightValue
* @param {import('postcss').Declaration} decl
* @param {string} weightValue
* @param {import('postcss-value-parser').Node} [weightValueNode]
*/
function checkWeight(weightValue, decl) {
function checkWeight(decl, weightValue, weightValueNode) {
if (!isStandardSyntaxValue(weightValue)) {
return;
}
Expand All @@ -98,73 +103,108 @@ const rule = (primary, secondaryOptions) => {
return;
}

if (
weightValue.toLowerCase() === INHERIT_KEYWORD ||
weightValue.toLowerCase() === INITIAL_KEYWORD
) {
if (includesOnlyFunction(weightValue)) {
return;
}

const lowerWeightValue = weightValue.toLowerCase();

if (lowerWeightValue === INHERIT_KEYWORD || lowerWeightValue === INITIAL_KEYWORD) {
return;
}

if (
optionsMatches(secondaryOptions, 'ignore', 'relative') &&
keywordSets.fontWeightRelativeKeywords.has(weightValue.toLowerCase())
keywordSets.fontWeightRelativeKeywords.has(lowerWeightValue)
) {
return;
}

const weightValueOffset = decl.value.indexOf(weightValue);

if (primary === 'numeric') {
const parent = decl.parent;

if (parent && isAtRule(parent) && parent.name.toLowerCase() === 'font-face') {
const weightValueNumbers = postcss.list.space(weightValue);

if (!weightValueNumbers.every((value) => isNumbery(value))) {
return complain(messages.expected('numeric'));
// @font-face allows multiple values.
for (const valueNode of findFontWeights(weightValue)) {
if (!isNumbery(valueNode.value)) {
return complain(messages.expected('numeric'), valueNode);
}
}

return;
}

if (!isNumbery(weightValue)) {
return complain(messages.expected('numeric'));
return complain(messages.expected('numeric'), weightValueNode);
}
}

if (primary === 'named-where-possible') {
if (isNumbery(weightValue)) {
if (WEIGHTS_WITH_KEYWORD_EQUIVALENTS.has(weightValue)) {
complain(messages.expected('named'));
complain(messages.expected('named'), weightValueNode);
}

return;
}

if (
!keywordSets.fontWeightKeywords.has(weightValue.toLowerCase()) &&
weightValue.toLowerCase() !== NORMAL_KEYWORD
!keywordSets.fontWeightKeywords.has(lowerWeightValue) &&
lowerWeightValue !== NORMAL_KEYWORD
) {
return complain(messages.invalidNamed(weightValue));
return complain(messages.invalidNamed(weightValue), weightValueNode);
}
}

/**
* @param {string} message
* @param {import('postcss-value-parser').Node} [valueNode]
*/
function complain(message) {
function complain(message, valueNode) {
const index = declarationValueIndex(decl) + (valueNode ? valueNode.sourceIndex : 0);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[note] I'll add endIndex on #5725.


report({
ruleName,
result,
message,
node: decl,
index: declarationValueIndex(decl) + weightValueOffset,
index,
});
}
}
};
};

/**
* @param {string} value
* @returns {import('postcss-value-parser').Node[]}
*/
function findFontWeights(value) {
return valueParser(value).nodes.filter((node, index, nodes) => {
if (node.type !== 'word') return false;

// Exclude `<font-size>/<line-height>` format like `16px/3`.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const prevNode = nodes[index - 1];
const nextNode = nodes[index + 1];

if (prevNode && prevNode.type === 'div') return false;

if (nextNode && nextNode.type === 'div') return false;

return true;
});
}

/**
* @param {string} value
* @returns {boolean}
*/
function includesOnlyFunction(value) {
return valueParser(value).nodes.every(({ type }) => {
return type === 'function' || type === 'comment' || type === 'space';
});
}

rule.ruleName = ruleName;
rule.messages = messages;
rule.meta = meta;
Expand Down