Add "time-min-milliseconds" rule #2289

hudochenkov · 2017-01-25T20:48:11Z

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!

jeddy3 · 2017-01-26T12:46:17Z

lib/rules/time-min-milliseconds/__tests__/index.js

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


Can we get a test for 0.1s please?

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

jeddy3 · 2017-01-26T12:46:35Z

lib/rules/time-min-milliseconds/__tests__/index.js

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


Can we get a test for 100ms please?

davidtheclark

Thanks! Left a few comments.

davidtheclark · 2017-01-26T14:39:17Z

docs/user-guide/rules.md

@@ -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.


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

davidtheclark · 2017-01-26T14:39:46Z

lib/rules/time-min-milliseconds/README.md

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


number of milliseconds

davidtheclark · 2017-01-26T14:40:51Z

lib/rules/time-min-milliseconds/README.md

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


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.

davidtheclark · 2017-01-26T14:43:32Z

lib/rules/time-min-milliseconds/__tests__/index.js

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


Missing hyphen, I think.

davidtheclark · 2017-01-26T14:45:18Z

lib/rules/time-min-milliseconds/__tests__/index.js

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


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?

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?

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.

davidtheclark · 2017-01-26T14:46:36Z

lib/rules/time-min-milliseconds/index.js

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


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

davidtheclark · 2017-01-26T14:47:18Z

lib/rules/time-min-milliseconds/index.js

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


These two conditions could be combined into one if statement.

davidtheclark · 2017-01-26T14:48:40Z

lib/rules/time-min-milliseconds/index.js

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


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

hudochenkov · 2017-01-26T16:06:55Z

@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.

davidtheclark · 2017-01-28T18:05:07Z

lib/rules/time-min-milliseconds/index.js

@@ -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`,


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

hudochenkov · 2017-01-28T18:24:14Z

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

davidtheclark · 2017-01-28T19:02:25Z

@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.

davidtheclark · 2017-01-29T17:34:27Z

Added to Changelog:

Added: time-min-milliseconds rule, to replace time-no-imperceptible (#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

Add "time-min-milliseconds" rule

0a359c8

hudochenkov added the PR: review needed label Jan 25, 2017

hudochenkov mentioned this pull request Jan 26, 2017

Deprecate additional rules #2285

Merged

jeddy3 requested changes Jan 26, 2017

View reviewed changes

Fix warnings if time is equals to config time

ec3c631

davidtheclark requested changes Jan 26, 2017

View reviewed changes

Fixes based on a feedback

a8174a7

davidtheclark requested changes Jan 28, 2017

View reviewed changes

Fix typo

ffee81f

Check only positive numbers

e91f6ce

jeddy3 approved these changes Jan 29, 2017

View reviewed changes

jeddy3 mentioned this pull request Jan 29, 2017

Deprecate rules #2123

Closed

13 tasks

davidtheclark approved these changes Jan 29, 2017

View reviewed changes

davidtheclark merged commit 0464b2d into master Jan 29, 2017

davidtheclark deleted the time-min-milliseconds branch January 29, 2017 17:33

jeddy3 added PR: review accepted and removed PR: review needed labels Jan 29, 2017

jeddy3 mentioned this pull request Jan 31, 2017

Add time-min-milliseconds rule #2249

Closed

keylocation-bot mentioned this pull request Feb 2, 2017

Update dependency stylelint to version 7.8.0 singapore/lint-condo#230

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "time-min-milliseconds" rule #2289

Add "time-min-milliseconds" rule #2289

hudochenkov commented Jan 25, 2017

jeddy3 left a comment

jeddy3 Jan 26, 2017

hudochenkov Jan 26, 2017

jeddy3 Jan 26, 2017

davidtheclark left a comment

davidtheclark Jan 26, 2017

davidtheclark Jan 26, 2017

davidtheclark Jan 26, 2017

davidtheclark Jan 26, 2017

davidtheclark Jan 26, 2017

hudochenkov Jan 26, 2017

davidtheclark Jan 28, 2017

davidtheclark Jan 26, 2017

davidtheclark Jan 26, 2017

davidtheclark Jan 26, 2017

hudochenkov commented Jan 26, 2017

davidtheclark left a comment

davidtheclark Jan 28, 2017

hudochenkov Jan 28, 2017

hudochenkov commented Jan 28, 2017 •

edited

davidtheclark commented Jan 28, 2017

davidtheclark commented Jan 29, 2017

		@@ -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.


		## Options

		`int`: Minimum number milliseconds for time values.

Add "time-min-milliseconds" rule #2289

Add "time-min-milliseconds" rule #2289

Conversation

hudochenkov commented Jan 25, 2017

jeddy3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidtheclark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hudochenkov commented Jan 26, 2017

davidtheclark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hudochenkov commented Jan 28, 2017 • edited

davidtheclark commented Jan 28, 2017

davidtheclark commented Jan 29, 2017

hudochenkov commented Jan 28, 2017 •

edited