-
-
Notifications
You must be signed in to change notification settings - Fork 929
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 ignoreUnits option to number-max-precision rule #2941
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sendilkumarn Thanks for this!
I've made a couple of test and docs requests as the option also accepts regexs.
Can you also remove the CHANGELOG item (as we do that after the PR is merged to avoid conflicts) and the yarn.lock
file.
Note: let's add yarn.lock
to .gitignore
in another PR.
@@ -37,3 +37,46 @@ a { top: 3.24px; } | |||
```css | |||
@media (min-width: 3.23em) {} | |||
``` | |||
## Optional secondary options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add one blank line above the heading.
(Note: Let's see if remark can lint for this).
@@ -37,3 +37,46 @@ a { top: 3.24px; } | |||
```css | |||
@media (min-width: 3.23em) {} | |||
``` | |||
## Optional secondary options | |||
|
|||
### `ignoreUnits: ["string"]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The optionsMatches
util accepts regular expressions as well as strings. Can you update the examples to reflect this please, like so? e.g.
["/^my-/", "%"]
a { top: 3.245my-unit; }
|
||
testRule(rule, { | ||
ruleName, | ||
config: [0, { ignoreUnits: ["%"] }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get update the config to:
ignoreUnits: ["%", "/^my-/"]
And get an accept and reject test for the "/^my-/"
value.
520b777
to
d22eabb
Compare
@jeddy3 done the changes. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sendilkumarn Thanks for making the changes (and for the other PRs!).
I've two very minor doc requests, then I think this is ready.
|
||
## Optional secondary options | ||
|
||
### `ignoreUnits: ["string"]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### ignoreUnits: ["/regex/", "string"]
|
||
### `ignoreUnits: ["string"]` | ||
|
||
Ignore number-max-precision rule for the value with the unit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the following description, please:
"Ignore the precision of numbers for values with the specified units."
As it's more consistent with the descriptions else where.
adding regex option fixing comments
d22eabb
to
e31a988
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sendilkumarn Thanks for making the changes. LGTM.
|
#2785
No, it's self explanatory