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 "time-min-milliseconds" rule #2289

Merged
merged 5 commits into from Jan 29, 2017

Conversation

3 participants
@hudochenkov
Member

hudochenkov commented Jan 25, 2017

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

#2249

Is there anything in the PR that needs further explanation?

No, it's self explanatory.

@jeddy3

@hudochenkov Thanks for this!

config: [MIN_VALUE],
accept: [ {
code: "a { transition-delay: 0.2s; }",

This comment has been minimized.

@jeddy3

jeddy3 Jan 26, 2017

Member

Can we get a test for 0.1s please?

This comment has been minimized.

@hudochenkov

hudochenkov Jan 26, 2017

Member

Great catch! It was failing for 0.1s and 100ms :)

}, {
code: "a { transition-delay: 3s; }",
}, {
code: "a { transition-delay: 200ms; }",

This comment has been minimized.

@jeddy3

jeddy3 Jan 26, 2017

Member

Can we get a test for 100ms please?

@davidtheclark

Thanks! Left a few comments.

@@ -62,6 +62,7 @@ Here are all the rules within stylelint, grouped by the [*thing*](http://apps.wo
### Time
- [`time-min-milliseconds`](../../lib/rules/time-min-milliseconds/README.md): Limit the minimum number of milliseconds for time values.

This comment has been minimized.

@davidtheclark

davidtheclark Jan 26, 2017

Contributor

I think "Specify" is a better word here than "Limit": "Specify the minimum number ..."

## Options
`int`: Minimum number milliseconds for time values.

This comment has been minimized.

@davidtheclark

davidtheclark Jan 26, 2017

Contributor

number of milliseconds

```
```css
a { animation: horse-dance 1s linear 0.01s; }

This comment has been minimized.

@davidtheclark

davidtheclark Jan 26, 2017

Contributor

I find this to be an ambiguous example. A reader might reasonably come away thinking both 1s and 0.01s are not ok. Let's just have one time value in each example, since no text specifies which particular value is the violator.

Same applies to the example below.

}, {
code: "a { animation-duration: 0.15s; }",
}, {
code: "a { -webkitanimation-duration: 0.15s; }",

This comment has been minimized.

@davidtheclark

davidtheclark Jan 26, 2017

Contributor

Missing hyphen, I think.

}, {
code: "a { animation: foo 0.8s 200ms ease-in-out; }",
}, {
code: "a { animation-delay: -2.5s; }",

This comment has been minimized.

@davidtheclark

davidtheclark Jan 26, 2017

Contributor

This is a little confusing and needs to be clarified in the docs. Do any negative values work? Or is the "minimum" actually an absolute value applying on both sides of zero?

This comment has been minimized.

@hudochenkov

hudochenkov Jan 26, 2017

Member

Good question! I think we should check only positive values.

According to specs transition-duration and animation-duration can't be negative. Negative values can be in shorthand properties transition and animation, but it's hard to parse what time is which in shorthands.

Also, negative delay is a rare case. Developer could set negative value in two cases: intentionally for some reason, or by mistake.

What do you think?

This comment has been minimized.

@davidtheclark

davidtheclark Jan 28, 2017

Contributor

Yes, I agree: if transition-duration and animation-duration cannot be negative, then let's not use the absolute value here, and only check positive values.

const ruleName = "time-min-milliseconds"
const messages = ruleMessages(ruleName, {
expected: time => `Expected no less than ${time} milliseconds`,

This comment has been minimized.

@davidtheclark

davidtheclark Jan 26, 2017

Contributor

Let's say Expected of a minimum of ${time} milliseconds to avoid quarrels about "less" vs "fewer".

root.walkDecls(decl => {
if (keywordSets.longhandTimeProperties.has(postcss.vendor.unprefixed(decl.prop.toLowerCase()))) {
if (isNotAcceptableTime(decl.value)) {

This comment has been minimized.

@davidtheclark

davidtheclark Jan 26, 2017

Contributor

These two conditions could be combined into one if statement.

}
function complain(decl) {
const offset = arguments.length > 1 && arguments[1] !== undefined ? arguments[1] : 0

This comment has been minimized.

@davidtheclark

davidtheclark Jan 26, 2017

Contributor

Ugh this is awkward. Can you use an explicit argument instead?

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jan 26, 2017

@davidtheclark Thank you for an excellent review. To be honest, I just took an old rule and changed few things to be a new rule ¯\_(ツ)_/¯ Will cleanup things.

@davidtheclark

One wording comment; and we also need changes to rules/index.js and the example config when adding a new rule.

@@ -12,7 +12,7 @@ const valueParser = require("postcss-value-parser")
const ruleName = "time-min-milliseconds"
const messages = ruleMessages(ruleName, {
expected: time => `Expected no less than ${time} milliseconds`,
expected: time => `Expected of a minimum of ${time} milliseconds`,

This comment has been minimized.

@davidtheclark

davidtheclark Jan 28, 2017

Contributor

Cut the first "of" here: Expected a minimum of ${time} ...

This comment has been minimized.

@hudochenkov
@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jan 28, 2017

There is an important question lost in outdated diff comments. What numbers should this rule check? And my opinion about it.

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Jan 28, 2017

@hudochenkov I responded above, then realized it might be lost in the diff, too:

Yes, I agree: if transition-duration and animation-duration cannot be negative, then let's not use the absolute value here, and only check positive values.

@jeddy3

jeddy3 approved these changes Jan 29, 2017

@jeddy3 jeddy3 referenced this pull request Jan 29, 2017

Closed

Deprecate rules #2123

13 of 13 tasks complete

@davidtheclark davidtheclark merged commit 0464b2d into master Jan 29, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@davidtheclark davidtheclark deleted the time-min-milliseconds branch Jan 29, 2017

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Jan 29, 2017

Added to Changelog:

  • Added: time-min-milliseconds rule, to replace time-no-imperceptible (#2289).

sergesemashko pushed a commit to sergesemashko/stylelint that referenced this pull request Mar 3, 2017

Add "time-min-milliseconds" rule (stylelint#2289)
* Add "time-min-milliseconds" rule

* Fix warnings if time is equals to config time

* Fixes based on a feedback

* Fix typo

* Check only positive numbers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment