Skip to content
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

no-vendor-prefixes : Throws error on Non-Standard Rule #702

Closed
egaudette opened this issue May 11, 2016 · 11 comments · Fixed by #709
Closed

no-vendor-prefixes : Throws error on Non-Standard Rule #702

egaudette opened this issue May 11, 2016 · 11 comments · Fixed by #709

Comments

@egaudette
Copy link

egaudette commented May 11, 2016

When enforcing the no-vendor-prefixes rule, it throws errors due to non-standard rules, as utilized in normalize.css:

/* Remove inner padding and border in Firefox 4+.*/
button::-moz-focus-inner, input::-moz-focus-inner { border: 0; padding: 0; }

/* Fix the cursor style for Chrome's increment/decrement buttons. For certain font-size values of the input, it causes the cursor style of the decrement button to change from default` to text.*/

input[type="number"]::-webkit-inner-spin-button, input[type="number"]::-webkit-outer-spin-button { height: auto; }

error Vendor prefixes should not be used no-vendor-prefixes

Yes, this is a vendor prefix, but it is for a non-standard rule, and is targeting Specific Browser Sudo Elements

I'm tempted to exclude our normalize.scss file from linting, but I would prefer not to.

Could there be an option, added to the rule, that would allow the user to list non-standard css rules that would then be ignored by the linter?

What version of Sass Lint are you using?
"gulp-sass": "2.0.4"
"gulp-sass-lint": "1.7.0"

@egaudette egaudette changed the title -moz-focus-inner throws error. no-vendor-prefixes : Throws error on Non-Standard Rule May 12, 2016
@DanPurdy
Copy link
Member

DanPurdy commented May 12, 2016

@egaudette @keeganstreet How about rather than a whitelist we check if it's a standard property by default? We already have a list of those for other rules. This way you wont need to whitelist them and we don't need to care about them. We could even have the allow-non-standard option if you'd like some control which would be default set to true?

If the only way you can use these properties is with the vendor prefixes then a whitelist may seem like too much maintenance? Open to suggestions around that though.

@egaudette
Copy link
Author

Dan, I'll admit that I'm not up to date with the existing of all the non-standard rules, and I'm not sure how you're utilizing them within the linting package. If you have a list of those, and can automatically skip them during validation of this rule, that would be great!

I will have to see what you propose, and test it out, before being able to visualize all the edge use cases. If you want me to test out a feature branch, I'd surely be happy to do so.

Thanks!

@DanPurdy
Copy link
Member

Ok I'll have a little look in a bit then and see what I think.

We have a complete list of the standard properties and we also have a property spelling rule that allows whitelisting of extra property names so I would imagine a combination of that rule and the ignore non standard properties proposal here would cover you both for incorrect property names and the use of non standard ones.

I'll ping you when i have a PR ready 👍

@egaudette
Copy link
Author

egaudette commented May 12, 2016

OK, so I imagine that the no-vendor-prefixes rule does a search for the the prefix values themselves, and doesn't take the actual strings in the remainder of the rules into account at all?

Therefore, the rule needs to be augmented to:

  • Check for properties for vendor prefixes.

    -- If Prefix

    --- If !cssProperties.indexOf(property) && !property.whitelisted

    ---- throw error.

Or something to that manor of flow.

@DanPurdy
Copy link
Member

You can try the rule out now from #709 I've chosen to leave the rule functioning as is for now with the option to ignore-non-standard properties down to the user. I felt that it made too much of a drastic change in the number of warnings reported that it may need to be disabled by default for now.

Open to opinions here though.

@egaudette
Copy link
Author

@DanPurdy Sorry for the delayed response, and again, forgive my ignorance, but how would I test this locally? I have cloned the repo, where I have seen your changes reflected in the "develop" branch. I then tried to point our "gulp-sass-lint" reference to the local repo, as opposed to providing a version for npm to install, within the package.json file.

https://docs.npmjs.com/files/package.json#local-paths

I didn't get this working correctly. I am sure that this new feature is sufficient for our needs, and I wouldn't want to hold up the development/deployment of this feature.

Still, if you would like for me to give this a once over, I would more than happy to do so. If so, could you point me in the direction of documentation that would allow for testing of this?

Thanks, and best regards,
-Ethan

@DanPurdy
Copy link
Member

@egaudette sorry for the long delay in releasing this but 1.8.0 is out now with the these changes included.

@egaudette
Copy link
Author

@DanPurdy No worries! Thanks for the ping. I'll give them a once over in the coming weeks.

Thanks again for all the hard work, guys.

@keeganstreet
Copy link

Thanks!

@egaudette
Copy link
Author

@DanPurdy Sorry for the day. The 'ignore-non-standard: true' is working out great!

Yet, I just tried to utilize this improvement, but I am running into a problem with a -ms- prefixed property (gold old IE - never dies). Take the following mixin, for example. Since IE8 takes a different format for its 'filter' property, it needs the -ms- prefix. If the -ms- prefix is added to the excluded-identifiers array, then all -ms- rules are ignored, where I only need to ignore the filter property.

// Provides cross-browser CSS opacity. Takes a number between 0 and 1 as the argument. Ex: @include opacity(0.8);
// Note: to reduce code bloat, remove the any unneeded browser support below
@mixin opacity($opacity) {
    $opacity-ie: $opacity * 100;
    /* IE 8 */
   -ms-filter: "progid:DXImageTransform.Microsoft.Alpha(Opacity=" + $opacity-ie + ")";
    /* IE 5-7 */
    filter: alpha(opacity = $opacity-ie);
    /* Good browsers */
    opacity: $opacity;
}

Can/should this rule be extended to allow an array if ignored properties, independent of any identifiers?

Am I misinterpreting how the IE version would evaluate the filter property?

@WillsB3
Copy link

WillsB3 commented Feb 16, 2018

I'm having trouble getting the ignore-non-standard setting working. I'm sure I'm doing something wrong...

Here's my config:

# This config extends sass-lint's default rule set which can be found at
# https://github.com/sasstools/sass-lint/blob/develop/lib/config/sass-lint.yml.
options:
  merge-default-rules: true

rules:
  # Mixins
  mixins-before-declarations:
    - 1
    -
      exclude:
      - 'mq'

  # Name Formats
  class-name-format:
    - 2
    -
      convention: hyphenatedbem
  function-name-format:
    - 1
    -
      convention: hyphenatedlowercase

  no-vendor-prefixes:
    - 1
    -
      ignore-non-standard: true

  # Style Guide
  nesting-depth:
    - 1
    -
      max-depth: 3
  no-css-comments: 0
  no-warn: 0

Any idea what might be the issue?

Thanks 👍

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

Successfully merging a pull request may close this issue.

4 participants