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

Ignore some properties with 0 value (#4250) #4394

Merged
merged 5 commits into from Nov 9, 2019
Merged

Ignore some properties with 0 value (#4250) #4394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c7d8086
Ignore some properties with 0 value (#4250)
fanich37 Nov 1, 2019
4f405c6
Fix prettier issue
fanich37 Nov 2, 2019
947abe7
Update tests
fanich37 Nov 2, 2019
26f02ae
Remove array with grid props
fanich37 Nov 6, 2019
7b31f62
Add logic to process font decl, update tests
fanich37 Nov 8, 2019
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
131 changes: 131 additions & 0 deletions lib/rules/length-zero-no-unit/__tests__/index.js
View file
Open in desktop
Expand Up @@ -183,6 +183,65 @@ testRule(rule, {
code: 'a { margin : 0; }',
description: 'space before and after colon',
},
{
code: 'a { line-height : 0px; }',
description: 'ignore line-height property',
},
{
code: 'a { lIne-hEight: 0px; }',
},
{
code: 'a { grid-auto-columns: 0fr; }',
description: 'ignore some grid properties with `fr`',
},
{
code: 'a { grid-Auto-cOlUmns: 0fr; }',
description: 'ignore some grid properties with `fr`',
},
{
code: 'a { grid-template-columns: repeat(10, 0fr); }',
description: 'ignore some grid properties with `fr`',
},
{
code: 'a { Grid-TEMplate-cOlumns : repeat( 10, 0fr ); }',
description: 'ignore some grid properties with `fr`',
},
{
code: 'a { grid-template-rows: 0fr 1fr; }',
description: 'ignore some grid properties with `fr`',
},
{
code: 'a { grId-tEMplate-Rows:0fr 1fr; }',
description: 'ignore some grid properties with `fr`',
},
{
code: 'a { font: normal normal 400 16px/0px cursive; }',
description: 'ignore line-height in font declaration',
},
{
code: 'a { font: normal normal 400 16px/0 cursive; }',
description: 'ignore line-height in font declaration',
},
{
code: 'a { font: normal normal 400 0 / 0px cursive; }',
description: 'ignore line-height in font declaration',
},
{
code: 'a { font: normal normal 400 16px / 0px cursive; }',
description: 'ignore line-height in font declaration',
},
{
code: 'a { font: normal normal 400 16px/ 0px cursive; }',
description: 'ignore line-height in font declaration',
},
{
code: 'a { font: normal normal 400 16px /0px cursive; }',
description: 'ignore line-height in font declaration',
},
{
code: 'a { font: normal normal 400 1.2em cursive; }',
description: 'do not fail if no line-height in font declaration',
},
],

reject: [
Expand Down Expand Up @@ -400,6 +459,78 @@ testRule(rule, {
line: 1,
column: 11,
},
{
code: 'a { grid-template-columns: 0px 0fr 1fr };',
fixed: 'a { grid-template-columns: 0 0fr 1fr };',

message: messages.rejected,
line: 1,
column: 29,
},
{
code: 'a { grid-template-colUMns: 0px 0fr 1fr };',
fixed: 'a { grid-template-colUMns: 0 0fr 1fr };',

message: messages.rejected,
line: 1,
column: 29,
},
{
code: 'a { grid-template-rows: 40px 4fr 0px; };',
fixed: 'a { grid-template-rows: 40px 4fr 0; };',

message: messages.rejected,
line: 1,
column: 35,
},
{
code: 'a { grid-auto-columns: 0px };',
fixed: 'a { grid-auto-columns: 0 };',

message: messages.rejected,
line: 1,
column: 25,
},
{
code: 'a { grid-template-columns: repeat(2, 50px 0px) 100px; };',
fixed: 'a { grid-template-columns: repeat(2, 50px 0) 100px; };',

message: messages.rejected,
line: 1,
column: 44,
},
{
code: 'a { font: normal normal 400 0px / 0px cursive; }',
fixed: 'a { font: normal normal 400 0 / 0px cursive; }',

message: messages.rejected,
line: 1,
column: 30,
},
{
code: 'a { font: normal normal 400 0px /0px cursive; }',
fixed: 'a { font: normal normal 400 0 /0px cursive; }',

message: messages.rejected,
line: 1,
column: 30,
},
{
code: 'a { font: normal normal 400 0px/0px cursive; }',
fixed: 'a { font: normal normal 400 0/0px cursive; }',

message: messages.rejected,
line: 1,
column: 30,
},
{
code: 'a { font: normal normal 400 0px/ 0px cursive; }',
fixed: 'a { font: normal normal 400 0/ 0px cursive; }',

message: messages.rejected,
line: 1,
column: 30,
},
],
});

Expand Down
23 changes: 21 additions & 2 deletions lib/rules/length-zero-no-unit/index.js
View file
Open in desktop
Expand Up @@ -29,11 +29,26 @@ const rule = function(actual, secondary, context) {
}

root.walkDecls((decl) => {
if (decl.prop.toLowerCase() === 'line-height') {
Copy link
Member

Choose a reason for hiding this comment

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

Not only line-height can contains <length> | <percentage>, for example https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/stroke-width, https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/stroke-dashoffset and other, you can find them here https://github.com/mdn/data/blob/master/css/properties.json, also let's put them in https://github.com/stylelint/stylelint/tree/master/lib/reference

Copy link
Member Author

Choose a reason for hiding this comment

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

To be honest I completely lost the idea why I'd ignored line-height 😕 .
This is the same:

line-height: 0;
line-height: 0px;
line-height: 0em;

So in case of zero value the dimension unit is useless and the warning should be flagged (or it should be auto-fixed).
The only unit to ignore here is fr since 0fr and 0 mean completely different things.

Copy link
Member

Choose a reason for hiding this comment

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

line-height: 0; !== line-height: 0px;

Сlarifications and examples - cssnano/cssnano#768

Copy link
Member Author

Choose a reason for hiding this comment

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

Make sense.
But what the point to ignore stroke-width for example. It doesn't matter whether it has unit or not. Example: https://jsfiddle.net/vc2otk06/

Copy link
Member

Choose a reason for hiding this comment

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

Oh, yes, my mistake, you are right, don't found other properties with same ability as line-height has, let's add tests for this cases:

a { font: normal normal 400 16px/0px cursive; }
a { font: normal normal 400 16px/0 cursive; }

Copy link
Member Author

@fanich37 fanich37 Nov 8, 2019 •
edited

Choose a reason for hiding this comment

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

The funny thing is these cases don't break tests in master branch but they have to.
I added some logic to process font declaration and updated test cases.

return;
}

const stringValue = blurComments(decl.toString());
const ignorableIndexes = new Array(stringValue.length).fill(false);
const parsedValue = valueParser(stringValue);

parsedValue.walk((node) => {
parsedValue.walk((node, nodeIndex) => {
if (decl.prop.toLowerCase() === 'font' && node.type === 'div' && node.value === '/') {
const lineHeightNode = parsedValue.nodes[nodeIndex + 1];
const lineHeightNodeValue = valueParser.stringify(lineHeightNode);

for (let i = 0; i < lineHeightNodeValue.length; i++) {
ignorableIndexes[lineHeightNode.sourceIndex + i] = true;
}

return;
}

if (node.type !== 'function') {
return;
}
Expand Down Expand Up @@ -97,7 +112,7 @@ const rule = function(actual, secondary, context) {
const valueWithZeroStart = prevValueBreakIndex === -1 ? 0 : prevValueBreakIndex + 1;

const nextValueBreakIndex = _.findIndex(value.substr(valueWithZeroStart), (char) => {
return [' ', ',', ')'].indexOf(char) !== -1;
return [' ', ',', ')', '/'].indexOf(char) !== -1;
});

// If no next break was found, this value ends at the end of the string
Expand All @@ -111,6 +126,10 @@ const rule = function(actual, secondary, context) {
return;
}

if (parsedValue.unit.toLowerCase() === 'fr') {
return;
}

// Add the indexes to ignorableIndexes so the same value will not
// be checked multiple times.
_.range(valueWithZeroStart, valueWithZeroEnd).forEach((i) => (ignorableIndexes[i] = true));
Expand Down