Add font-family-no-missing-generic-family-keyword #2930

stoyan · 2017-09-28T21:29:57Z

This addresses issue (feature request) #2929. I mostly copied font-family-no-duplicate-names and tweaked accordingly.

To test I ran:

$ npm test lib/rules/font-family-generic-fallback/tests/

Not sure how to improve the coverage of the tests. Results:

Test Suites: 1 passed, 1 total
Tests: 22 passed, 22 total
Snapshots: 0 total
Time: 2.609s
Ran all test suites matching /lib/rules/font-family-generic-fallback/tests//i.

=============================== Coverage summary ===============================
Statements : 36.9% ( 2955/8008 )
Branches : 4.85% ( 184/3794 )
Functions : 5.15% ( 72/1398 )
Lines : 37.12% ( 2950/7947 )

Jest: Coverage for statements (36.9%) does not meet global threshold (75%)
Jest: Coverage for branches (4.85%) does not meet global threshold (75%)
Jest: Coverage for lines (37.12%) does not meet global threshold (75%)
Jest: Coverage for functions (5.15%) does not meet global threshold (75%)
npm ERR! Test failed. See above for more details.

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

#2929

Is there anything in the PR that needs further explanation?

I don't think so

This addresses issue stylelint#2929 ran $ npm test lib/rules/font-family-generic-fallback/__tests__/ Not sure how to improve the coverage of the tests. Results: Test Suites: 1 passed, 1 total Tests: 22 passed, 22 total Snapshots: 0 total Time: 2.609s Ran all test suites matching /lib\/rules\/font-family-generic-fallback\/__tests__\//i. =============================== Coverage summary =============================== Statements : 36.9% ( 2955/8008 ) Branches : 4.85% ( 184/3794 ) Functions : 5.15% ( 72/1398 ) Lines : 37.12% ( 2950/7947 ) ================================================================================ Jest: Coverage for statements (36.9%) does not meet global threshold (75%) Jest: Coverage for branches (4.85%) does not meet global threshold (75%) Jest: Coverage for lines (37.12%) does not meet global threshold (75%) Jest: Coverage for functions (5.15%) does not meet global threshold (75%) npm ERR! Test failed. See above for more details.

CAYdenberg · 2017-09-29T02:04:42Z

Hey thanks for your idea and your work.

It's often a good idea to leave your issue up for a bit and see if there is any discussion ... the core is quite lively with hashing out issues and often things will be spotted that can avoid a chunk of work.

Having said that your tests do pass and your code runs fine: I don't think our coverage suite is really set up to report on one file at a time? If run npm test everything looks good (takes about 96 sec) ... while working you can use npm run watch, hit the p key, and then zero in on your test file using a regular expression.

I'll take a look at the code itself depending on the discussion surrounding the feature request.

stoyan · 2017-09-30T20:27:40Z

Thanks! Yeah, I poked around the code, and then read up on the procedures for pull request. My bad :)

CAYdenberg · 2017-10-01T02:11:34Z

NP. I like the idea and think this will get merged eventually.

jeddy3

@stoyan Thanks! This does look good.

Once we settle on the the rule name, we can amend the rule description etc accordingly.

In the meantime, I've just some initial minor requests.

jeddy3 · 2017-10-01T12:24:20Z

lib/rules/font-family-generic-fallback/README.md

+
+```css
+a { font-family: Arial, sans-serif; }
+/**                        ↑


Align the arrow to the start of the generic family

jeddy3 · 2017-10-01T12:25:54Z

lib/rules/font-family-generic-fallback/README.md

+The following patterns are *not* considered violations:
+
+```css
+a { font-family: Times, serif; }


Let's match these first two examples to the violations above, but fixed. I think that'll make it clearer.

jeddy3 · 2017-10-01T12:28:27Z

lib/rules/font-family-generic-fallback/index.js

+        return;
+      }
+
+      complain(


No need for separate out the report into a complain function when it is only called once.

CAYdenberg · 2017-10-02T02:45:08Z

lib/rules/font-family-generic-fallback/index.js

+
+const isFamilyNameKeyword = node =>
+  !node.quote && keywordSets.fontFamilyKeywords.has(node.value.toLowerCase());
+


Does !node.quote mean anything in quotes will NOT be seen as a generic family name? I would have assumed that font-family: Times, "serif", is perfectly valid and should pass, although it is slightly weird.

According to the spec, generic font families must be unquoted:

The following generic family keywords are defined: ‘serif’, ‘sans-serif’, ‘cursive’, ‘fantasy’, and ‘monospace’. These keywords can be used as a general fallback mechanism when an author's desired font choices are not available. As keywords, they must not be quoted. Authors are encouraged to append a generic font family as a last alternative for improved robustness.

So, the code as it is LGTM.

I stand corrected ... agreed.

CAYdenberg · 2017-10-02T02:46:46Z

lib/rules/font-family-generic-fallback/README.md

@@ -0,0 +1,39 @@
+# font-family-generic-fallback
+


As described in the associated issue, please change all references to the rule name to font-family-no-missing-generic.

SGTM. As there seems to be no objections to the name proposal, @stoyan can you rename this font-family-no-missing-generic.

jeddy3

@stoyan Thanks again for starting this. I believe its a rule all stylelint users will find useful. Also, thank you for your patience with the review. We've all been busy with other things.

There doesn't appear to be any objections to the naming proposal. Therefore, I've added some further requests to make the rule consistent with the conventions in stylelint. With 150 rules in stylelint we endeavour to be consistent, to help both the users and maintainers of stylelint. Thanks for your understanding.

The final step is to wire-up-the-rule. Can you add the rule to the example config please and to the list of rules (under possible errors -> font family)

jeddy3 · 2017-10-03T08:21:37Z

lib/rules/font-family-generic-fallback/README.md

@@ -0,0 +1,39 @@
+# font-family-generic-fallback
+


SGTM. As there seems to be no objections to the name proposal, @stoyan can you rename this font-family-no-missing-generic.

jeddy3 · 2017-10-03T08:26:46Z

lib/rules/font-family-generic-fallback/README.md

@@ -0,0 +1,39 @@
+# font-family-generic-fallback
+
+Require a generic fallback to any font-family declaration.


Can we change this to the following, as this is now a no rules:

Disallow missing generic families in lists of font family names.

jeddy3 · 2017-10-03T08:30:11Z

lib/rules/font-family-generic-fallback/index.js

+const ruleName = "font-family-generic-fallback";
+
+const messages = ruleMessages(ruleName, {
+  generic: () => "Missing generic font family, e.g. serif"


For no rules, the convention is to use rejected. A simple string would suffice too. Can you please use:

rejected: "Unexpected missing generic font family"

You'll need to update your tests accordingly too.

jeddy3 · 2017-10-03T08:32:58Z

lib/rules/index.js

@@ -52,6 +52,7 @@ const declarationPropertyUnitBlacklist = require("./declaration-property-unit-bl
 const declarationPropertyUnitWhitelist = require("./declaration-property-unit-whitelist");
 const declarationPropertyValueBlacklist = require("./declaration-property-value-blacklist");
 const declarationPropertyValueWhitelist = require("./declaration-property-value-whitelist");
+const fontFamilyGenericFallback = require("./font-family-generic-fallback");


Please update the rule name.

stoyan · 2017-10-04T02:14:01Z

Thanks folks, this is awesome! I'll work on it as soon as I can.

... and other assorted review feedback

jeddy3

@stoyan Thanks! This looks great.

As a "possible errors" rule, it'll also be included in config-recommended (and config-standard), which means quite a few people will be using it from the get-go.

jeddy3 · 2017-10-22T08:39:40Z

Changelog:

Added: font-family-no-missing-generic-family-keyword rule (#2930).

jeddy3 requested changes Oct 1, 2017

View reviewed changes

CAYdenberg suggested changes Oct 2, 2017

View reviewed changes

jeddy3 requested changes Oct 3, 2017

View reviewed changes

jeddy3 added the PR: needs revision label Oct 3, 2017

jeddy3 changed the title ~~new rule font-family-generic-fallback~~ Add font-family-no-missing-generic Oct 3, 2017

font-family-no-missing-generic-family-keyword rename

a397e28

... and other assorted review feedback

CAYdenberg approved these changes Oct 22, 2017

View reviewed changes

jeddy3 approved these changes Oct 22, 2017

View reviewed changes

jeddy3 added PR: review needed and removed PR: needs revision labels Oct 22, 2017

jeddy3 changed the title ~~Add font-family-no-missing-generic~~ Add font-family-no-missing-generic-family-keyword Oct 22, 2017

jeddy3 merged commit 3df2f61 into stylelint:master Oct 22, 2017

jeddy3 mentioned this pull request Nov 4, 2017

Add font-family-no-missing-generic #2929

Closed

jsoref mentioned this pull request Nov 27, 2017

Spelling sumyapp/basecamp3_slack_notify#22

Merged

dependencies bot mentioned this pull request Dec 5, 2017

Update 23 dependencies from npm JetBrains/ring-ui#31

Merged

dependencies bot mentioned this pull request Dec 18, 2017

Update stylelint in / from 8.2.0 to 8.4.0 chauncey-garrett/dotfiles#62

Open

dependencies bot mentioned this pull request Dec 27, 2017

Update stylelint in / from 8.2.0 to 8.4.0 dailybruin/sources#22

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add font-family-no-missing-generic-family-keyword #2930

Add font-family-no-missing-generic-family-keyword #2930

stoyan commented Sep 28, 2017 •

edited

Loading

CAYdenberg commented Sep 29, 2017 •

edited

Loading

stoyan commented Sep 30, 2017

CAYdenberg commented Oct 1, 2017

jeddy3 left a comment

jeddy3 Oct 1, 2017

jeddy3 Oct 1, 2017

jeddy3 Oct 1, 2017

CAYdenberg Oct 2, 2017

jeddy3 Oct 2, 2017

CAYdenberg Oct 2, 2017

CAYdenberg Oct 2, 2017

jeddy3 Oct 3, 2017

jeddy3 left a comment •

edited

Loading

jeddy3 Oct 3, 2017

jeddy3 Oct 3, 2017

jeddy3 Oct 3, 2017

jeddy3 Oct 3, 2017

stoyan commented Oct 4, 2017

jeddy3 left a comment

jeddy3 commented Oct 22, 2017


		const isFamilyNameKeyword = node =>
		!node.quote && keywordSets.fontFamilyKeywords.has(node.value.toLowerCase());

		@@ -0,0 +1,39 @@
		# font-family-generic-fallback

		Require a generic fallback to any font-family declaration.

Add font-family-no-missing-generic-family-keyword #2930

Add font-family-no-missing-generic-family-keyword #2930

Conversation

stoyan commented Sep 28, 2017 • edited Loading

=============================== Coverage summary =============================== Statements : 36.9% ( 2955/8008 ) Branches : 4.85% ( 184/3794 ) Functions : 5.15% ( 72/1398 ) Lines : 37.12% ( 2950/7947 )

CAYdenberg commented Sep 29, 2017 • edited Loading

stoyan commented Sep 30, 2017

CAYdenberg commented Oct 1, 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

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

jeddy3 left a comment • edited Loading

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

stoyan commented Oct 4, 2017

jeddy3 left a comment

Choose a reason for hiding this comment

jeddy3 commented Oct 22, 2017

stoyan commented Sep 28, 2017 •

edited

Loading

=============================== Coverage summary ===============================
Statements : 36.9% ( 2955/8008 )
Branches : 4.85% ( 184/3794 )
Functions : 5.15% ( 72/1398 )
Lines : 37.12% ( 2950/7947 )

CAYdenberg commented Sep 29, 2017 •

edited

Loading

jeddy3 left a comment •

edited

Loading