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.

Sign up for GitHub

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

Jump to bottom

Fix shorthand-property-no-redundant-values false negatives for functions #7657

Merged
merged 7 commits into from Apr 27, 2024
Merged

Fix shorthand-property-no-redundant-values false negatives for functions #7657

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
000f631
Fix `shorthand-property-no-redundant-values` false negatives for func…
ybiquitous Apr 26, 2024
f6cf12b
Apply suggestion for readability
ybiquitous Apr 26, 2024
7739172
build
ybiquitous Apr 26, 2024
47ddc79
Add test case for `border-radius` with `/`
ybiquitous Apr 26, 2024
7ab186d
Exclude `var()` support
ybiquitous Apr 26, 2024
101003d
Refactor to use `return` instead of `break` in loop
ybiquitous Apr 27, 2024
b290aee
Refactor to rename `isShorthandProperty` to `isSupportedShorthand`
ybiquitous Apr 27, 2024
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
5 changes: 5 additions & 0 deletions .changeset/famous-deers-build.md
View file
Open in desktop
@@ -0,0 +1,5 @@
---
"stylelint": minor
---

Fixed: `shorthand-property-no-redundant-values` false negatives for functions
29 changes: 21 additions & 8 deletions lib/rules/shorthand-property-no-redundant-values/__tests__/index.mjs
View file
Open in desktop
Expand Up @@ -78,23 +78,19 @@ testRule({
description: 'ignore not shorthandable property',
},
{
code: 'a { border-radius: 1px / 1px }',
code: 'a { border-radius: 1px / 1px; }',
description: 'ignore ellipse',
},
ybiquitous marked this conversation as resolved.
Show resolved Hide resolved
{
code: 'a { margin: calc(1px + 1px) calc(1px + 1px); }',
description: 'ignore calc function',
},
{
code: 'a { margin: some-function(1px + 1px) some-function(1px + 1px); }',
description: 'ignore all function',
code: 'a { border-radius: 1px 1px / 1px; }',
description: 'ignore ellipse',
},
{
code: 'a { margin: var(--margin) var(--margin); }',
description: 'ignore variables',
},
{
code: 'a { border-color: #FFFFFF transparent transparent }',
code: 'a { border-color: #FFFFFF transparent transparent; }',
description: 'ignore upper case value',
},
{
Expand Down Expand Up @@ -381,6 +377,23 @@ testRule({
line: 1,
column: 5,
},
{
code: 'a { margin: calc(1px + 1px) calc(1px + 1px); }',
fixed: 'a { margin: calc(1px + 1px); }',
message: messages.rejected('calc(1px + 1px) calc(1px + 1px)', 'calc(1px + 1px)'),
line: 1,
column: 5,
},
{
code: 'a { margin: some-function(1px + 1px) some-function(1px + 1px); }',
fixed: 'a { margin: some-function(1px + 1px); }',
message: messages.rejected(
'some-function(1px + 1px) some-function(1px + 1px)',
'some-function(1px + 1px)',
),
line: 1,
column: 5,
},
Mouvedia marked this conversation as resolved.
Show resolved Hide resolved
],
});

Expand Down
62 changes: 33 additions & 29 deletions lib/rules/shorthand-property-no-redundant-values/index.cjs
View file
Open in desktop
Expand Up @@ -5,10 +5,12 @@
const valueParser = require('postcss-value-parser');
const isStandardSyntaxDeclaration = require('../../utils/isStandardSyntaxDeclaration.cjs');
const isStandardSyntaxProperty = require('../../utils/isStandardSyntaxProperty.cjs');
const isStandardSyntaxValue = require('../../utils/isStandardSyntaxValue.cjs');
const report = require('../../utils/report.cjs');
const ruleMessages = require('../../utils/ruleMessages.cjs');
const validateOptions = require('../../utils/validateOptions.cjs');
const vendor = require('../../utils/vendor.cjs');
const typeGuards = require('../../utils/typeGuards.cjs');

const ruleName = 'shorthand-property-no-redundant-values';

Expand All @@ -32,16 +34,6 @@ const propertiesWithShorthandNotation = new Set([
'inset',
]);

const ignoredCharacters = ['+', '*', '/', '(', ')', '$', '@', '--', 'var('];

/**
* @param {string} value
* @returns {boolean}
*/
function hasIgnoredCharacters(value) {
return ignoredCharacters.some((char) => value.includes(char));
}

/**
* @param {string} property
* @returns {boolean}
Expand Down Expand Up @@ -125,40 +117,52 @@ const rule = (primary, _secondaryOptions, context) => {
}

root.walkDecls((decl) => {
if (!isStandardSyntaxDeclaration(decl) || !isStandardSyntaxProperty(decl.prop)) {
return;
}
if (!isStandardSyntaxDeclaration(decl)) return;

const { prop, value } = decl;

const prop = decl.prop;
const value = decl.value;
if (!isStandardSyntaxProperty(prop)) return;

if (!isStandardSyntaxValue(value)) return;

const normalizedProp = vendor.unprefixed(prop.toLowerCase());

if (hasIgnoredCharacters(value) || !isShorthandProperty(normalizedProp)) {
return;
}
if (!isShorthandProperty(normalizedProp)) return;

/** @type {string[]} */
const valuesToShorthand = [];

valueParser(value).walk((valueNode) => {
if (valueNode.type !== 'word') {
return;
let shouldIgnore = false;

for (const valueNode of valueParser(value).nodes) {
if (typeGuards.isValueDiv(valueNode)) {
shouldIgnore = true;
break;
}

valuesToShorthand.push(valueParser.stringify(valueNode));
});
// `var()` should be ignored. E.g.
//
// --padding: 20px 5px;
// padding: 0 var(--padding) 0;
//
if (typeGuards.isValueFunction(valueNode) && valueNode.value.toLowerCase() === 'var') {
shouldIgnore = true;
break;
}

if (typeGuards.isValueWord(valueNode) || typeGuards.isValueFunction(valueNode)) {
valuesToShorthand.push(valueParser.stringify(valueNode));
}
}

if (shouldIgnore) return;

if (valuesToShorthand.length <= 1 || valuesToShorthand.length > 4) {
return;
}

const shortestForm = canCondense(
valuesToShorthand[0] || '',
valuesToShorthand[1] || '',
valuesToShorthand[2] || '',
valuesToShorthand[3] || '',
);
const [top, right, bottom, left] = valuesToShorthand;
const shortestForm = canCondense(top ?? '', right ?? '', bottom ?? '', left ?? '');
const shortestFormString = shortestForm.filter(Boolean).join(' ');
const valuesFormString = valuesToShorthand.join(' ');

Expand Down
63 changes: 34 additions & 29 deletions lib/rules/shorthand-property-no-redundant-values/index.mjs
View file
Open in desktop
Expand Up @@ -2,11 +2,14 @@ import valueParser from 'postcss-value-parser';

import isStandardSyntaxDeclaration from '../../utils/isStandardSyntaxDeclaration.mjs';
import isStandardSyntaxProperty from '../../utils/isStandardSyntaxProperty.mjs';
import isStandardSyntaxValue from '../../utils/isStandardSyntaxValue.mjs';
import report from '../../utils/report.mjs';
import ruleMessages from '../../utils/ruleMessages.mjs';
import validateOptions from '../../utils/validateOptions.mjs';
import vendor from '../../utils/vendor.mjs';

import { isValueDiv, isValueFunction, isValueWord } from '../../utils/typeGuards.mjs';

const ruleName = 'shorthand-property-no-redundant-values';

const messages = ruleMessages(ruleName, {
Expand All @@ -29,16 +32,6 @@ const propertiesWithShorthandNotation = new Set([
'inset',
]);
Mouvedia marked this conversation as resolved.
Show resolved Hide resolved

const ignoredCharacters = ['+', '*', '/', '(', ')', '$', '@', '--', 'var('];

/**
* @param {string} value
* @returns {boolean}
*/
function hasIgnoredCharacters(value) {
return ignoredCharacters.some((char) => value.includes(char));
}

/**
* @param {string} property
* @returns {boolean}
Expand Down Expand Up @@ -122,40 +115,52 @@ const rule = (primary, _secondaryOptions, context) => {
}

root.walkDecls((decl) => {
if (!isStandardSyntaxDeclaration(decl) || !isStandardSyntaxProperty(decl.prop)) {
return;
}
if (!isStandardSyntaxDeclaration(decl)) return;

const { prop, value } = decl;

const prop = decl.prop;
const value = decl.value;
if (!isStandardSyntaxProperty(prop)) return;
Mouvedia marked this conversation as resolved.
Show resolved Hide resolved

if (!isStandardSyntaxValue(value)) return;

const normalizedProp = vendor.unprefixed(prop.toLowerCase());

if (hasIgnoredCharacters(value) || !isShorthandProperty(normalizedProp)) {
return;
}
if (!isShorthandProperty(normalizedProp)) return;
ybiquitous marked this conversation as resolved.
Show resolved Hide resolved

/** @type {string[]} */
const valuesToShorthand = [];

valueParser(value).walk((valueNode) => {
if (valueNode.type !== 'word') {
return;
let shouldIgnore = false;

for (const valueNode of valueParser(value).nodes) {
if (isValueDiv(valueNode)) {
shouldIgnore = true;
break;
}

valuesToShorthand.push(valueParser.stringify(valueNode));
});
// `var()` should be ignored. E.g.
//
// --padding: 20px 5px;
// padding: 0 var(--padding) 0;
//
if (isValueFunction(valueNode) && valueNode.value.toLowerCase() === 'var') {
shouldIgnore = true;
break;
}
ybiquitous marked this conversation as resolved.
Show resolved Hide resolved

if (isValueWord(valueNode) || isValueFunction(valueNode)) {
valuesToShorthand.push(valueParser.stringify(valueNode));
}
}

if (shouldIgnore) return;

if (valuesToShorthand.length <= 1 || valuesToShorthand.length > 4) {
return;
}

const shortestForm = canCondense(
valuesToShorthand[0] || '',
valuesToShorthand[1] || '',
valuesToShorthand[2] || '',
valuesToShorthand[3] || '',
);
const [top, right, bottom, left] = valuesToShorthand;
Mouvedia marked this conversation as resolved.
Show resolved Hide resolved
const shortestForm = canCondense(top ?? '', right ?? '', bottom ?? '', left ?? '');
const shortestFormString = shortestForm.filter(Boolean).join(' ');
const valuesFormString = valuesToShorthand.join(' ');

Expand Down
9 changes: 9 additions & 0 deletions lib/utils/typeGuards.cjs
View file
Open in desktop
Expand Up @@ -53,6 +53,14 @@ function isDocument(node) {
return node.type === 'document';
}

/**
* @param {import('postcss-value-parser').Node} node
* @returns {node is import('postcss-value-parser').DivNode}
*/
function isValueDiv(node) {
return node.type === 'div';
}

/**
* @param {import('postcss-value-parser').Node} node
* @returns {node is import('postcss-value-parser').FunctionNode}
Expand Down Expand Up @@ -92,6 +100,7 @@ exports.isDeclaration = isDeclaration;
exports.isDocument = isDocument;
exports.isRoot = isRoot;
exports.isRule = isRule;
exports.isValueDiv = isValueDiv;
exports.isValueFunction = isValueFunction;
exports.isValueSpace = isValueSpace;
exports.isValueWord = isValueWord;
8 changes: 8 additions & 0 deletions lib/utils/typeGuards.mjs
View file
Open in desktop
Expand Up @@ -49,6 +49,14 @@ export function isDocument(node) {
return node.type === 'document';
}

/**
* @param {import('postcss-value-parser').Node} node
* @returns {node is import('postcss-value-parser').DivNode}
*/
export function isValueDiv(node) {
Mouvedia marked this conversation as resolved.
Show resolved Hide resolved
return node.type === 'div';
}

/**
* @param {import('postcss-value-parser').Node} node
* @returns {node is import('postcss-value-parser').FunctionNode}
Expand Down