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 media-feature-name-value-whitelist #3320

Merged
merged 12 commits into from Jun 12, 2018

Conversation

3 participants
@MatthiasKunnen
Member

MatthiasKunnen commented May 19, 2018

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

Closes #3312

Is there anything in the PR that needs further explanation?

No, it's self explanatory.

MatthiasKunnen added some commits May 18, 2018

@MatthiasKunnen MatthiasKunnen force-pushed the MatthiasKunnen:media-feature-name-value-whitelist branch from 9c2de90 to bf806df May 19, 2018

@MatthiasKunnen MatthiasKunnen referenced this pull request May 28, 2018

Closed

Release 9.3 #3319

4 of 4 tasks complete
@evilebottnawi

Thanks for work 👍 Can you add tests to avoid non standard syntax problem (you can look on this in other same rules)?

@MatthiasKunnen

This comment has been minimized.

Member

MatthiasKunnen commented May 29, 2018

@evilebottnawi, there are two tests for non standard syntax. SCSS in particular. What kind of additional tests do you have in mind?

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented May 29, 2018

@stylelint/contributors we ignore non standard syntax in any places right?

@MatthiasKunnen

This comment has been minimized.

Member

MatthiasKunnen commented May 29, 2018

Correct

@MatthiasKunnen

This comment has been minimized.

Member

MatthiasKunnen commented May 29, 2018

Non standard syntax is handled as suggested in the issue (#3312 (comment)) by @hudochenkov.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented May 29, 2018

/cc @hudochenkov we do same for same rules?

@jeddy3

@MatthiasKunnen Thanks for this! It's looking great.

I've made a few minor requests.

(Sorry about the delay reviewing this. I've been out the country for a couple of weeks.)


Non standard syntax is handled as suggested in the issue (#3312 (comment)) by @hudochenkov.

@evilebottnawi /cc @hudochenkov we do same for same rules?

I believe this rule is behaving consistently to other whitelist and blacklist rules.

@@ -171,6 +171,7 @@ Here are all the rules within stylelint, grouped first [by category](../../VISIO
- [`media-feature-name-blacklist`](../../lib/rules/media-feature-name-blacklist/README.md): Specify a blacklist of disallowed media feature names.
- [`media-feature-name-no-vendor-prefix`](../../lib/rules/media-feature-name-no-vendor-prefix/README.md): Disallow vendor prefixes for media feature names.
- [`media-feature-name-value-whitelist`](../../lib/rules/media-feature-name-value-whitelist/README.md): Specify a whitelist of allowed media feature and value pairs.

This comment has been minimized.

@jeddy3

jeddy3 Jun 4, 2018

Member

A minor tweak, as I believe the "name" and the "value" represent the pair.

Specify a whitelist of allowed media feature name and value pairs.

@@ -0,0 +1,65 @@
# media-feature-name-value-whitelist
Specify a whitelist of allowed media feature and value pairs.

This comment has been minimized.

@jeddy3

jeddy3 Jun 4, 2018

Member

Specify a whitelist of allowed media feature name and value pairs.

```css
@media screen and (min-width: 768px) {}
/** ↑ ↑

This comment has been minimized.

@jeddy3

jeddy3 Jun 4, 2018

Member

Please align the arrows with the start of each bit of vocab i.e. the "m" in "min-width" and "7" in "768px"

(There are a lot of rules in stylelint, so we try to be consistent as possible across all the docs).

This comment has been minimized.

@MatthiasKunnen

MatthiasKunnen Jun 4, 2018

Member

Will do. I've not come across this in the docs so I wasn't aware of this rule.

This comment has been minimized.

@jeddy3

jeddy3 Jun 5, 2018

Member

Yep, it's not an obvious one to glean. I've created a PR to explicitly highlight the convention (along with the "This rule ..." one).

* These features and these values */
```
**Caveat:** Media feature names within range or boolean context are ignored.

This comment has been minimized.

@jeddy3

jeddy3 Jun 4, 2018

Member

Please write as the following to be consistent with most of the other rules:

"This rule ignores media features within range or boolean context."

I'll create a separate issue about updating the other rules that still use the old "Caveat:" convention (e.g. media-feature-name-blacklist).

This comment has been minimized.

@MatthiasKunnen

MatthiasKunnen Jun 4, 2018

Member

Yeah, I reused some things here and there to the rule as consistent as possible. That the caveat was deprecated, I did not know :)

This comment has been minimized.

@jeddy3

jeddy3 Jun 5, 2018

Member

Yeah, I reused some things here and there to the rule as consistent as possible.

Yep, I saw. Good job and thanks :)

That the caveat was deprecated, I did not know :)

We're still ironing out all the kinks in the docs, which is why your kind of feedback is so useful.

We just merged a PR to make the other rule READMEs consistent.

}
```
If a media feature is found, only its whitelisted property values are allowed. The rule reports all

This comment has been minimized.

@jeddy3

jeddy3 Jun 4, 2018

Member

Let's go with the following to be more consistent with declaration-property-value-whitelist:


Options

object: { "unprefixed-media-feature-name": ["array", "of", "values"] }

If a media feature name is found in the object, only its whitelisted values are allowed. If the media feature name is not included in the object, anything goes.

If a name or value is surrounded with / (e.g. "/width$/"), it is interpreted as a regular expression. For example, /width$/ will match max-width and min-width.


This comment has been minimized.

@MatthiasKunnen

MatthiasKunnen Jun 4, 2018

Member

Is this the only rule that uses object:? I personally find it atrocious to look at/read in code as well as rendered. The JSON object is better formatted.

{
  "max-width": ["Array", "of", "values"],
  "/regex/": ["/regex/", "non-regex"]
}

I believe there is another rule that uses this in some form but I'd have to check.

This comment has been minimized.

@jeddy3

jeddy3 Jun 5, 2018

Member

I believe there is another rule that uses this in some form but I'd have to check.

I think it's only the declaration-property-*-*list rules that accept objects. I might be wrong, though.

I personally find it atrocious to look at/read in code as well as rendered.

Yep, I was thinking the same thing. However, if we can't find another example of it done as a JSON object then I suggest we go with the other approach in this PR to keep things consistent.

We should then create a separate issue to address how we can better communicate the options across the board. To be honest, all the option descriptions need a little love and I don't think we ever rationalised the approach we took. It'd be good to look, as part of that issue, into how other tools communicate their options.

If a property or value is surrounded with `/` (e.g. `"/width$/"`), it is interpreted as a regular
expression. For example, `/width$/` will match `max-width` and `min-width`.
## Examples

This comment has been minimized.

@jeddy3

jeddy3 Jun 4, 2018

Member

Remove this title, please.

## Examples
Given

This comment has been minimized.

@jeddy3

jeddy3 Jun 4, 2018

Member

"Given:"

(It's a little clunky, but the stylelint website needs the semicolon for the pattern background colours).

```json
{
"min-width": ["768px"],

This comment has been minimized.

@jeddy3

jeddy3 Jun 4, 2018

Member

I think two values here would be useful to the reader e.g. "min-width": ["768px", "1024px"].

You'll need to add an additional pattern to the "not considered violations" list for it.

column: 31
},
{
code: "@media screen (min-width: 768px) and (min-width: 1000px) {}",

This comment has been minimized.

@jeddy3

jeddy3 Jun 4, 2018

Member

Can we get this two lines please to test the line property value e.g.

code: "@media screen (min-width: 768px)\nand (min-width: 1000px) {}",`
description: "Media feature multiple",
message: messages.rejected("min-width", "1000px"),
line: 2,
column: 5

This comment has been minimized.

@MatthiasKunnen

MatthiasKunnen Jun 6, 2018

Member

You mean at column 17, right? The rule reports errors on the value, not the media feature name.

This comment has been minimized.

@jeddy3

jeddy3 Jun 7, 2018

Member

You mean at column 17, right?

Oops, yes.

const ruleName = "media-feature-name-value-whitelist";
const messages = ruleMessages(ruleName, {

This comment has been minimized.

@jeddy3

jeddy3 Jun 4, 2018

Member

Formatters will usually append the rule name, therefore the "media feature" bit is a little redundant. Let's go with the following instead:

const messages = ruleMessages(ruleName, {
  rejected: (name, value) =>
    `Unexpected value "${value}" for name "${name}"`
});
@MatthiasKunnen

This comment has been minimized.

Member

MatthiasKunnen commented Jun 4, 2018

@jeddy3, thank you for the review. I've replied on some of your requested changes and will push updates soon.

@evilebottnawi, you've requested changes but that seems to be out-of-date.

@MatthiasKunnen MatthiasKunnen force-pushed the MatthiasKunnen:media-feature-name-value-whitelist branch from 2fd77ab to d55d3cf Jun 6, 2018

MatthiasKunnen added some commits Jun 6, 2018

@MatthiasKunnen MatthiasKunnen force-pushed the MatthiasKunnen:media-feature-name-value-whitelist branch from d55d3cf to 4059aa0 Jun 6, 2018

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 7, 2018

@MatthiasKunnen Changes look good!

I think once the line test is in and the options section is changed to:


Options

object: { "unprefixed-media-feature-name": ["array", "of", "values"] }

If a media feature name is found in the object, only its whitelisted values are allowed. If the media feature name is not included in the object, anything goes.

If a name or value is surrounded with / (e.g. "/width$/"), it is interpreted as a regular expression. For example, /width$/ will match max-width and min-width.


We'll be good to merge and get this into 9.3.0

@MatthiasKunnen

This comment has been minimized.

Member

MatthiasKunnen commented Jun 7, 2018

@jeddy3, the line test is already added. I suggest to resolve #3372 first. I already have a PR ready but I am holding on to it until my last comment has been addressed.

MatthiasKunnen added some commits Jun 11, 2018

@MatthiasKunnen

This comment has been minimized.

Member

MatthiasKunnen commented Jun 11, 2018

@jeddy3, this PR is ready I believe. The new options format in the README depends on PR #3389.

@jeddy3

jeddy3 approved these changes Jun 12, 2018

@MatthiasKunnen Thanks for making all the changes.

Excellent work. I'll merge this as-is, as #3372 has grown in scope and it'd be great to get this new rule into 9.3.

@jeddy3 jeddy3 merged commit d9e313a into stylelint:master Jun 12, 2018

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.02%) to 95.994%
Details
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 12, 2018

  • Added: media-feature-name-value-whitelist rule (#3320).

bors bot added a commit to mozilla/delivery-console that referenced this pull request Jun 15, 2018

Merge #194
194: Update dependency stylelint to v9.3.0 r=rehandalal a=renovate[bot]

This Pull Request updates dependency [stylelint](https://github.com/stylelint/stylelint) from `v9.2.1` to `v9.3.0`



<details>
<summary>Release Notes</summary>

### [`v9.3.0`](https://github.com/stylelint/stylelint/blob/master/CHANGELOG.md#&#8203;930)
[Compare Source](stylelint/stylelint@9.2.1...9.3.0)
-   Added: support for `<style>` tags and `style=""` attributes in XML and XSLT files ([#&#8203;3386](`stylelint/stylelint#3386)).
-   Added: `globbyOptions` option ([#&#8203;3339](`stylelint/stylelint#3339)).
-   Added: `keyframes-name-pattern` rule ([#&#8203;3321](`stylelint/stylelint#3321)).
-   Added: `media-feature-name-value-whitelist` rule ([#&#8203;3320](`stylelint/stylelint#3320)).
-   Added: `selector-pseudo-element-colon-notation` autofix ([#&#8203;3345](`stylelint/stylelint#3345)).
-   Fixed: `.vue` files throwing errors for `<style lang="stylus">` and `<style lang="postcss">` ([#&#8203;3331](`stylelint/stylelint#3331)).
-   Fixed: `declaration-block-no-*` false positives for non-standard syntax ([#&#8203;3381](`stylelint/stylelint#3381)).
-   Fixed: `function-whitespace-after` false positives for "/" ([#&#8203;3132](`stylelint/stylelint#3132)).
-   Fixed: `length-zero-no-unit` incorrect autofix for at-includes ([#&#8203;3347](`stylelint/stylelint#3347)).
-   Fixed: `max-nesting-depth` false positives for nested properties ([#&#8203;3349](`stylelint/stylelint#3349)).
-   Fixed: `no-empty-source` false positives on vue external sources `<style src="*">` tag ([#&#8203;3331](`stylelint/stylelint#3331)).
-   Fixed: `max-line-length` false positives for non-CSS blocks ([#&#8203;3367](`stylelint/stylelint#3367)).
-   Fixed: `no-eol-whitespace` false positives for non-CSS blocks ([#&#8203;3367](`stylelint/stylelint#3367)).
-   Fixed: `no-extra-semicolons` false positives for non-CSS blocks ([#&#8203;3367](`stylelint/stylelint#3367)).
-   Fixed: `no-missing-end-of-source-newline` false positives for non-CSS blocks ([#&#8203;3367](`stylelint/stylelint#3367)).

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>

schalkneethling added a commit to mdn/interactive-examples that referenced this pull request Jun 16, 2018

chore(deps): update dependency stylelint to v9.3.0 (#973)
This Pull Request updates dependency [stylelint](https://github.com/stylelint/stylelint) from `v9.2.1` to `v9.3.0`



<details>
<summary>Release Notes</summary>

### [`v9.3.0`](https://github.com/stylelint/stylelint/blob/master/CHANGELOG.md#&#8203;930)
[Compare Source](stylelint/stylelint@9.2.1...9.3.0)
-   Added: support for `<style>` tags and `style=""` attributes in XML and XSLT files ([#&#8203;3386](`stylelint/stylelint#3386)).
-   Added: `globbyOptions` option ([#&#8203;3339](`stylelint/stylelint#3339)).
-   Added: `keyframes-name-pattern` rule ([#&#8203;3321](`stylelint/stylelint#3321)).
-   Added: `media-feature-name-value-whitelist` rule ([#&#8203;3320](`stylelint/stylelint#3320)).
-   Added: `selector-pseudo-element-colon-notation` autofix ([#&#8203;3345](`stylelint/stylelint#3345)).
-   Fixed: `.vue` files throwing errors for `<style lang="stylus">` and `<style lang="postcss">` ([#&#8203;3331](`stylelint/stylelint#3331)).
-   Fixed: `declaration-block-no-*` false positives for non-standard syntax ([#&#8203;3381](`stylelint/stylelint#3381)).
-   Fixed: `function-whitespace-after` false positives for "/" ([#&#8203;3132](`stylelint/stylelint#3132)).
-   Fixed: `length-zero-no-unit` incorrect autofix for at-includes ([#&#8203;3347](`stylelint/stylelint#3347)).
-   Fixed: `max-nesting-depth` false positives for nested properties ([#&#8203;3349](`stylelint/stylelint#3349)).
-   Fixed: `no-empty-source` false positives on vue external sources `<style src="*">` tag ([#&#8203;3331](`stylelint/stylelint#3331)).
-   Fixed: `max-line-length` false positives for non-CSS blocks ([#&#8203;3367](`stylelint/stylelint#3367)).
-   Fixed: `no-eol-whitespace` false positives for non-CSS blocks ([#&#8203;3367](`stylelint/stylelint#3367)).
-   Fixed: `no-extra-semicolons` false positives for non-CSS blocks ([#&#8203;3367](`stylelint/stylelint#3367)).
-   Fixed: `no-missing-end-of-source-newline` false positives for non-CSS blocks ([#&#8203;3367](`stylelint/stylelint#3367)).

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment