Ignore some properties with 0 value (#4250) #4394
Conversation
|
This is strange. Rule file is not formatted by Prettier. But Prettier is run on pre-commit hook. Could you run In one of previous PRs you also had lint errors, because file wasn't formatted with Prettier. Maybe you skipping git hook intentionally? |
Hm... Hooks are installed but doesn't seem to work. Could it depend on an OS platform? I use Win 10 now and can also check it on Ubuntu. I also ran lint & tests manually before the commit and everything passed 😕 |
It shouldn't be a problem because of OS, but who knows, everything is possible. I have no idea what could be wrong :( |
| @@ -29,6 +29,10 @@ const rule = function(actual, secondary, context) { | |||
| } | |||
|
|
|||
| root.walkDecls((decl) => { | |||
| if (decl.prop.toLowerCase() === 'line-height') { | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
line-height: 0; !== line-height: 0px;
Сlarifications and examples - cssnano/cssnano#768
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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; }There was a problem hiding this comment.
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.
You are totally right, it simplifies work a lot =) |
|
It should close #4250 .
Nope, the logic of the PR is quite clear.