New rule: no-disallowed-property-values #1142
Conversation
| if (!disallowedPropertyValues) { | ||
| return; | ||
| } | ||
| if (typeof disallowedPropertyValues === 'string') { |
There was a problem hiding this comment.
You allow property values to be specified as strings outside of arrays here, you don't have this in your tests, it would be good to add this case in. Maybe duplicate the display: ['inline'] as just display: 'inline' that would help.
There was a problem hiding this comment.
Done! Though I ended up just replacing the display: ['inline'] case with display: 'inline' which feels like it should be sufficient?
| .foo { | ||
| text-transform: capitalize; | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we just add a quick example of multiple property values also, just to be complete in how we allow the property lists to be defined.
|
Hi @agnoster thanks for this, great stuff! Would you mind just having a look at the feedback I left, would be great to get this updated and ready for release soon. Also if you could just edit your initial comment to include your name/email for our contribution sign off |
|
closes #1019 |
…llowed-property-values * 'develop' of github.com:sasstools/sass-lint: Add updated CSS properties
|
Great comments! Updated the docs and tests for those cases - I think we actually don't need the "array with one value" case if we're testing "array with two values" and "single string value"? |
|
Hi @agnoster it would be good to include both. so single value as a string, single and multiple values in an array. It makes sense to test all the allowed cases like this just so we cover all cases. If you wouldn't mind just adding that case too then we could get this merged 👍 Its more so that in the future if we have to change something in the rule then we don't break compatibility for our users. |
|
Ah, fair enough, good reasoning! Done :-D |
|
What's the status of this PR? I'm hoping to use this rule in a project. Also, is there any plan to support blacklisting values that are strings? I noticed that if a property value is set to a string, |
|
Looks like it got approved but never merged? |
|
Sorry, our develop branch is now work for v2 and I just haven't had time to go back through everything and merge it all. I will hopefully in the next few days and release this. It will be part of our v2 update though which just removed the eslint dependencies etc. |
|
🎉 This PR is included in version 1.13.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
NOTE: I haven't done the DCO/Emoji stuff yet because I'm not sure if this is a desirable addition. If it is, I'll be happy to make any additional changes necessary to merge.
What do the changes you have made achieve?
This should allow blacklisting certain property values, in contrast to no-disallowed-properties which blacklists the property names. For example, this allows banning
text-transform: capitalizewhile allowing other uses oftext-transform.Are there any new warning messages?
Just the ones from the lint rule.
Have you written tests?
I have added tests based off the tests for
no-disallowed-properties.Have you included relevant documentation
I have added a new doc in
docs/rules/no-disallowed-property-values.mdWhich issues does this resolve?
No open issues that I found.
<DCO 1.1 Signed-off-by: Isaac Wolkerstorfer isaac@asana.com>