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

property-no-vendor-prefix: Add ignoreProperties #3089

Merged
merged 1 commit into from Jan 1, 2018

Conversation

4 participants
@nalinbhardwaj
Contributor

nalinbhardwaj commented Dec 30, 2017

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

#3007

Is there anything in the PR that needs further explanation?

No, it's self explanatory. Very similar to #3015.

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Dec 31, 2017

@stylelint/core Can someone else review this at their earliest convenience? This student is working with Google Code-In (GCI) and the PR needs to be merged in order for them to get credit for the assignment.

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jan 1, 2018

Thank you for the contribution!

I see few issues with documentation and tests. I'll do a proper review later today.

@nalinbhardwaj

This comment has been minimized.

Contributor

nalinbhardwaj commented Jan 1, 2018

@hudochenkov : I looked around in the repo myself to look for possible errors, and realised that other examples were consistent across the documentation page(and tests), perhaps that's the error you refer to, if so, I've fixed it. :)

EDIT: Seems the Travis build is failing on an unrelated test? And somehow the same tests(with node 8) pass on Appveyor(and offline for me)? Did I do something wrong?

@hudochenkov

I looked around in the repo myself to look for possible errors, and realised that other examples were consistent across the documentation page(and tests), perhaps that's the error you refer to, if so, I've fixed it. :)

Good catch! :)

EDIT: Seems the Travis build is failing on an unrelated test? And somehow the same tests(with node 8) pass on Appveyor(and offline for me)? Did I do something wrong?

Travis has some problems with its infrastructure currently. It's not a problem with your code.

code: "a { -moz-columns: 2; }"
},
{
code: ".foo { -webkit-animation-delay: 0.5s; }"

This comment has been minimized.

@hudochenkov

hudochenkov Jan 1, 2018

Member

Replace .foo with a in all tests.

{ ignoreProperties: ["transform", "columns", "/^animation-/"] }
],
accept: [

This comment has been minimized.

@hudochenkov

hudochenkov Jan 1, 2018

Member

Add a { -wEbKiT-tRaNsFoRm: scale(1); } accept test. That we know it's case insensitive.

Both config value case and property case should be case insensitive.

ignoreProperties: ["transform"] should match a { -webkit-TrAnSfOrM: none; }. And ignoreProperties: ["TrAnSfOrM"] should match a { -webkit-trAnsfOrm: none; }, etc.

Could you also add test for the later one, please?

This comment has been minimized.

@jeddy3

jeddy3 Jan 1, 2018

Member

Both config value case and property case should be case insensitive.

I believe this behaviour was changed in 8.0.0. Options are now case sensitive and optionsMatches will behave as such. Users should explicitly use the i RegExp flag for case-insensitivity e.g. ignoreProperties: ["/transform/i"].

So ignoreProperties: ["transform"] should not match a { -webkit-TrAnSfOrM: none; }.

It might be worth updating the config to { ignoreProperties: ["transform", "columns", "/^animation-/i"] } so that this behaviour can be confirmed in this rule i.e. animation-name and ANIMATION-name will both match, but TRANSFORM will not.

],
reject: [
{
code: "-webkit-border-radius: 10px;",

This comment has been minimized.

@hudochenkov

hudochenkov Jan 1, 2018

Member

Replace with a { -webkit-border-radius: 10px; }, because property outside a rule isn't a valid CSS.

This comment has been minimized.

@jeddy3

jeddy3 Jan 1, 2018

Member

@nalinbhardwaj FYI, this requirement is in the guide.

This comment has been minimized.

@nalinbhardwaj

nalinbhardwaj Jan 1, 2018

Contributor

Fixing. There are some more tests in the repo that violate this, example

column: 8
},
{
code: ".foo { -WEBKIT-tranSFoRM: translateY(-50%); }",

This comment has been minimized.

@hudochenkov

hudochenkov Jan 1, 2018

Member

This shouldn't be rejected because transform is in ignored properties.

This comment has been minimized.

@jeddy3

jeddy3 Jan 1, 2018

Member

See my comment above as why I believe this is valid.

if (!validOptions) {
return;
}
root.walkDecls(decl => {
const prop = decl.prop;
const prop = decl.prop,
unprefixedProp = postcss.vendor.unprefixed(prop);

This comment has been minimized.

@hudochenkov

hudochenkov Jan 1, 2018

Member

Use one const per assignment, please. We should've put a rule about this in our ESLint config.

@jeddy3

@nalinbhardwaj Looking good thanks. I've added a couple of comments.

{ ignoreProperties: ["transform", "columns", "/^animation-/"] }
],
accept: [

This comment has been minimized.

@jeddy3

jeddy3 Jan 1, 2018

Member

Both config value case and property case should be case insensitive.

I believe this behaviour was changed in 8.0.0. Options are now case sensitive and optionsMatches will behave as such. Users should explicitly use the i RegExp flag for case-insensitivity e.g. ignoreProperties: ["/transform/i"].

So ignoreProperties: ["transform"] should not match a { -webkit-TrAnSfOrM: none; }.

It might be worth updating the config to { ignoreProperties: ["transform", "columns", "/^animation-/i"] } so that this behaviour can be confirmed in this rule i.e. animation-name and ANIMATION-name will both match, but TRANSFORM will not.

## Optional secondary options
### `ignoreProperties: ["string"]`

This comment has been minimized.

@jeddy3

jeddy3 Jan 1, 2018

Member
### `ignoreProperties: ["/regex/", "string"]`

RegExp are also accepted.

],
reject: [
{
code: "-webkit-border-radius: 10px;",

This comment has been minimized.

@jeddy3

jeddy3 Jan 1, 2018

Member

@nalinbhardwaj FYI, this requirement is in the guide.

column: 8
},
{
code: ".foo { -WEBKIT-tranSFoRM: translateY(-50%); }",

This comment has been minimized.

@jeddy3

jeddy3 Jan 1, 2018

Member

See my comment above as why I believe this is valid.

@nalinbhardwaj

This comment has been minimized.

Contributor

nalinbhardwaj commented Jan 1, 2018

@jeddy3, @hudochenkov : Fixed.

@jeddy3

jeddy3 approved these changes Jan 1, 2018 edited

@nalinbhardwaj Thanks for the quick amends :)

LGTM. We can add non-string RegExp compatibility (rather than wrapped in a string) in #3008 once #3087 is in.

@hudochenkov

Thank you for your contribution!

Good luck with your assignment! :)

@jeddy3 jeddy3 merged commit e7da2a0 into stylelint:master Jan 1, 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.672%
Details

@nalinbhardwaj nalinbhardwaj deleted the nalinbhardwaj:property-no-vendor branch Jan 1, 2018

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jan 1, 2018

  • Added: ignoreProperties: [] to property-no-vendor-prefix (#3089).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment