Skip to content
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 font-family-no-missing-generic #2929

Closed
stoyan opened this issue Sep 28, 2017 · 15 comments
Closed

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

stoyan opened this issue Sep 28, 2017 · 15 comments
Labels
status: wip is being worked on by someone type: new rule an entirely new rule

Comments

@stoyan
Copy link
Contributor

stoyan commented Sep 28, 2017

Describe the issue. Is it a bug or a feature request (new rule, new option, etc.)?

This is a proposal for a new rule that always requires a generic font-family at the end of font-family list. Helps reduce various visual bugs when there's:

  • missing or disabled "web-safe" font,
  • misspelled font name
  • @font-face web font that failed to load

pull request coming in a minute

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

New rule called font-family-generic-fallback

What CSS is needed to reproduce this issue?

.foo {
  font-family: Arial;
}

What stylelint configuration is needed to reproduce this issue?

e.g.

{
  "rules": {
    "font-family-generic-fallback": "always"
  }
}

Which version of stylelint are you using?

fresh github branch

How are you running stylelint: CLI, PostCSS plugin, Node API?

Clone the repo, npm test

Does your issue relate to non-standard syntax (e.g. SCSS, nesting, etc.)?

No non-standard CSS

What did you expect to happen?

Warnings to be flagged when using the new rule

What actually happened (e.g. what warnings or errors you are getting)?

Nothing yet.

stoyan added a commit to stoyan/stylelint that referenced this issue Sep 28, 2017
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
Copy link
Contributor

I like the idea, personally. I think it's also consistent with the goals of stylelint.

@CAYdenberg CAYdenberg added the status: needs discussion triage needs further discussion label Sep 29, 2017
@stoyan
Copy link
Contributor Author

stoyan commented Sep 30, 2017

Forgot to mention http://www.phpied.com/psa-always-add-a-generic-font-family-backup/ has some recent examples (e.g. Bing search) where not following the rule acts up.

@jeddy3
Copy link
Member

jeddy3 commented Oct 1, 2017

I think it's also consistent with the goals of stylelint.

Agreed. I think it's an ideal candidate for a built-in rule.

New rule called font-family-generic-fallback

To adhere to the naming conventions I think it should be called something like font-family-no-missing-generic

  • font-family is the thing being targeted
  • no is used when disallowing something
  • missing is used for when something is optional (as the code is valid without it) but recommended.
  • generic can be the shorthand for the spec name for the thing within the thing i.e. "generic font family".

@CAYdenberg CAYdenberg added status: wip is being worked on by someone type: new rule an entirely new rule and removed status: needs discussion triage needs further discussion labels Oct 2, 2017
@jeddy3
Copy link
Member

jeddy3 commented Oct 2, 2017

Any objections to the rule name? If not, I'll add some additional feedback to the wip PR.

@jeddy3 jeddy3 changed the title Rule: require generic font-family Add font-family-no-missing-generic Oct 3, 2017
@stoyan
Copy link
Contributor Author

stoyan commented Oct 4, 2017

hm, turns out there's a legitimate use of other fonts after a generic.

before:

before

After

after

The Emoji font takes precedence over the generic, probably because of the unicode range in the font.

Actually you can test right here because github currently does this. Inspect this and delete all fonts preceding sans-serif: ♥

I wonder if we should keep the rule and let people opt out which is probably pretty rare.

@jeddy3
Copy link
Member

jeddy3 commented Oct 5, 2017

The Emoji font takes precedence over the generic, probably because of the unicode range in the font.

Interesting. I had no idea this was a thing.

I wonder if we should keep the rule and let people opt out which is probably pretty rare.

All of the built-in rules are opt-in. I agree that whether it's included in the recommended config or not should be discussed, though.

Two other approaches could be:

  • Loosen the default behaviour of the rule to check that a generic font family is used anywhere in the list, rather than just at the end.
  • Keep the default behaviour as it, but add a secondary optional option to trigger this behaviour e.g. allowNonTerminal: true.

There's nothing in the spec stating a generic family must come last, only this:

Authors are encouraged to append a generic font family as a last alternative for improved robustness.

So, it's not clear-cut which, if either, approach is best. What do you think?

@CAYdenberg
Copy link
Contributor

I think the first approach (check anywhere in the list) is the right one ... as a user nothing about font-family-no-missing-generic says the generic font has to be at the end (except common sense?). If there IS a generic font, it isn't missing ...

@stoyan
Copy link
Contributor Author

stoyan commented Oct 5, 2017

Example 3

again

Terminal generic (haha!) to me reads better and works the same (in this case). It's not like all the characters became emojis all of a sudden. Looks like it's all about the Unicode range defined in the font. But since, philosophically, Stylelint is unopinionated, if the spec is lax and the browsers are tolerant, the rule shouldn't enforce a preference.

If you have competing fonts that both share a Unicode range, and yet differ (picture a Venn diagram) and you want to play with which one takes precedence over the shared part, you should certainly be free to do so.

I'd vote #2. It's the same as #1 plus an opportunity to be more strict, if desired. Since the more relaxed check is the default, it cannot hurt, right?

@ntwb
Copy link
Member

ntwb commented Oct 5, 2017

Any objections to the rule name

Until I came here and read the rule description it wasn’t clear to me the purpose of this rule based on its current font-family-no-missing-generic proposed name.

@jeddy3
Copy link
Member

jeddy3 commented Oct 5, 2017

Interesting stuff.

I think the first approach (check anywhere in the list) is the right one ...If there IS a generic font, it isn't missing .

This makes sense to me.

Until I came here and read the rule description it wasn’t clear to me the purpose of this rule based on its current font-family-no-missing-generic proposed name.

Umm... any suggestions to what can be done about this? Would an extra family at the end help i.e. font-family-no-missing-generic-family where font-family is the thing and no-missing-generic-family is what is being checked?

@CAYdenberg
Copy link
Contributor

CAYdenberg commented Oct 5, 2017

How about font-family-no-missing-generic-fallback? It's a tad verbose, but maybe that's necessary?

@jeddy3
Copy link
Member

jeddy3 commented Oct 5, 2017

How about font-family-no-missing-generic-fallback? It's a tad verbose, but maybe that's necessary?

It's equally verbose as font-family-no-missing-generic-family :)

However, we've consistently used the actual spec terminology elsewhere in stylelint and so I'm rather stick with generic-family, rather than use generic-fallback as:

<generic-family> The following generic family keywords are defined: ‘serif’, ‘sans-serif’, ‘cursive’, ‘fantasy’, and ‘monospace’.

We could expand it to font-family-no-missing-generic-family-keyword if that's clearer?

@stoyan
Copy link
Contributor Author

stoyan commented Oct 5, 2017

❤️ font-family-no-missing-generic-family-keyword

A little on the verbose side but clear and correct. And self-documenting.

@jeddy3
Copy link
Member

jeddy3 commented Oct 5, 2017

A little on the verbose side but clear and correct. And self-documenting.

SGTM. Let's go with that.

@jeddy3
Copy link
Member

jeddy3 commented Nov 4, 2017

Done in #2930

@jeddy3 jeddy3 closed this as completed Nov 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: wip is being worked on by someone type: new rule an entirely new rule
Development

No branches or pull requests

4 participants