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

Add ignoreProperties option to value-keyword-case rule #2937

Merged
merged 6 commits into from Oct 5, 2017

Conversation

3 participants
@Bilie
Contributor

Bilie commented Oct 2, 2017

Which issue, if any, is this issue related to?

#2424

Is there anything in the PR that needs further explanation?

No, it's self explanatory.

@jeddy3

@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)) {

This comment has been minimized.

@jeddy3

jeddy3 Oct 3, 2017

Member

Please use the matchesStringOrRegExp util instead of .includes, like we do on this line. This will:

  1. Allow the option to accept regular expressions.
  2. Address the failing node@4 tests, as node@4 doesn't support .includes.
@@ -1938,3 +1938,79 @@ testRule(rule, {
}
]
});
testRule(rule, {

This comment has been minimized.

@jeddy3

jeddy3 Oct 3, 2017

Member

Please include a couple of regular expression tests, like we do for the ignoreKeywords option.

For example, with `"lower"`.
```js

This comment has been minimized.

@jeddy3

jeddy3 Oct 3, 2017

Member

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.

This comment has been minimized.

@Bilie

Bilie Oct 4, 2017

Contributor

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!

This comment has been minimized.

@jeddy3

jeddy3 Oct 4, 2017

Member

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.

This comment has been minimized.

@jeddy3

jeddy3 Oct 4, 2017

Member

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.

Biliana Valeva added some commits Oct 4, 2017

@jeddy3

@Bilie Thanks for incorporating my requests. It's looking good and almost there.

I've one or two more minor requests.

```css
a {
display: bloCk;
},

This comment has been minimized.

@jeddy3

jeddy3 Oct 4, 2017

Member

Extra comma

```
```css
div {

This comment has been minimized.

@jeddy3

jeddy3 Oct 4, 2017

Member
  1. Remove indent, please.
  2. Use the a selector, as per these guidelines.
}
```
```css

This comment has been minimized.

@jeddy3

jeddy3 Oct 4, 2017

Member
  1. Remove indent, please.
  2. Use the a selector, as per these guidelines.
code: "a { display: block; }"
},
{
code: "div { background: Red; }"

This comment has been minimized.

@jeddy3

jeddy3 Oct 4, 2017

Member

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.

Biliana Valeva added some commits Oct 4, 2017

code: "div { background: Red; }"
},
{
code: "div { Background: deepPink; }"

This comment has been minimized.

@jeddy3

jeddy3 Oct 4, 2017

Member

Don't forget these last two divs please :)

Biliana Valeva
@jeddy3

jeddy3 approved these changes Oct 4, 2017 edited

Thanks for making all the changes. This LGTM :)

(Note: Once this PR is in, I believe it should be refactored to use the optionsMatches util as so

@jeddy3 jeddy3 referenced this pull request Oct 5, 2017

Closed

Release 8.2.0 #2913

6 of 6 tasks complete

@jeddy3 jeddy3 merged commit 3457b84 into stylelint:master Oct 5, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 95.67%
Details
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Oct 5, 2017

  • Added: ignoreProperties option to value-keyword-case (#2937).
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Oct 5, 2017

@Bilie Thanks again for your first-time contribution. Your new option was just released in stylelint@8.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment