Add ignoreProperties option to value-keyword-case rule #2937
Conversation
There was a problem hiding this comment.
@Bilie Thank you very much for your first contribution. It's looking good.
The ignore* secondary options can usually accept regular expressions as well. I've made a couple of requests for this. Please don't hesitate to ask any questions you might have.
| @@ -164,6 +166,10 @@ const rule = function(expectation, options) { | |||
| return; | |||
| } | |||
|
|
|||
| if (ignoreProperties.length > 0 && ignoreProperties.includes(prop)) { | |||
There was a problem hiding this comment.
Please use the matchesStringOrRegExp util instead of .includes, like we do on this line. This will:
- Allow the option to accept regular expressions.
- Address the failing node@4 tests, as node@4 doesn't support
.includes.
| @@ -1938,3 +1938,79 @@ testRule(rule, { | |||
| } | |||
| ] | |||
| }); | |||
|
|
|||
| testRule(rule, { | |||
There was a problem hiding this comment.
Please include a couple of regular expression tests, like we do for the ignoreKeywords option.
|
|
||
| For example, with `"lower"`. | ||
|
|
||
| ```js |
There was a problem hiding this comment.
Please include some regular expression examples we do here for the ignoreKeywords option.
Also, can you please move the docs for this option above the ignoreKeywords option, as options should be listed alphabetically.
There was a problem hiding this comment.
Hi, thank you very much for your feedback! I was not sure if the new option should be called ignoreProperties or ignoreCustomProperties. Currently it is named ignoreProperties. Should it still go above ignoreKeywords in the readme or should I rename it to ignoreCustomProperties? Thank you!
There was a problem hiding this comment.
It should be called ignoreProperties. Custom properties are something different entirely in CSS. There might come a time when a user requests an ignoreCustomProperties option but that would be for the following example:
"value-keyword-case": ["lower", { ignoreCustomProperties: ["brand-color"] }]:root {
--brand-color: myMadeUPKeYwOrDColor;
}However, for what you're trying to achieve in this PR, ignoreProperties is the correct name.
There was a problem hiding this comment.
Also, can you please move the docs for this option above the ignoreKeywords option, as options should be listed alphabetically.
Whoops, I just realised my mistake here... K is before P in the alphabet... silly me. I can see why you were asking about custom properties now.
| ```css | ||
| a { | ||
| display: bloCk; | ||
| }, |
| ``` | ||
|
|
||
| ```css | ||
| div { |
There was a problem hiding this comment.
- Remove indent, please.
- Use the
aselector, as per these guidelines.
| } | ||
| ``` | ||
|
|
||
| ```css |
There was a problem hiding this comment.
- Remove indent, please.
- Use the
aselector, as per these guidelines.
| code: "a { display: block; }" | ||
| }, | ||
| { | ||
| code: "div { background: Red; }" |
There was a problem hiding this comment.
Use a rather than div for consistency please, otherwise a future maintainer might think the different selector is significant to the test when it is not.
| code: "div { background: Red; }" | ||
| }, | ||
| { | ||
| code: "div { Background: deepPink; }" |
There was a problem hiding this comment.
Don't forget these last two divs please :)
|
|
@Bilie Thanks again for your first-time contribution. Your new option was just released in |
#2424
No, it's self explanatory.