Add font-family-no-duplicate-names rule #2020
Conversation
| } | ||
| else { | ||
| familyNames.add(family) | ||
| } |
There was a problem hiding this comment.
Looks like the only difference between these two blocks is the value assigned to family. The code could probably be simplified significantly, right?
There was a problem hiding this comment.
@davidtheclark Not only family but also different Sets to store already found font family names. I use keywords to store found font family keywords (e.g. sans-serif, cursive, monospace ...). And I use familyNames to store other font family names. All this done in order to allow a { font-family: "sans-serif", sans-serif; } and don't throw an error.
There was a problem hiding this comment.
Ok, I guess I wasn't looking closely enough. I still think this code can be simplified, though. Please try to match the conventions in other parts of the codebase for brace spacing and early returns, and see if you can this up a little.
|
|
||
| This rule ignores `$sass`, `@less`, and `var(--custom-property)` variable syntaxes. | ||
|
|
||
| **Caveat:** This rule does not currently understand escape sequences. If you want to use the font family name "Hawaii 5-0" you will need to wrap it in quotes, instead of escaping it as `Hawaii \35 -0` or `Hawaii\ 5-0`. |
There was a problem hiding this comment.
If you want to use ...
This is inaccurate, right? You do not need to wrap it in quotes to use it. What is the actual consequence related to this rule of not wrapping the name in quotes?
There was a problem hiding this comment.
Not using quotes brings up a lot of ambiguity. For example,
Lucida GrandeLucida Grande"Lucida Grande""Lucida Grande"There was a problem hiding this comment.
Ok, so it sounds like the caveat is not about "escape sequences" but about quotation marks around multi-word font names?
There was a problem hiding this comment.
One can say so. The main idea - Please use quotation marks around multi-word font names or around font names containing disallowed symbols. Rather than use escape sequences in unquoted font family names. Because this rule does not currently support escape sequences.
Here some quotes from w3.org site:
Font family names other than generic families must either be given quoted as strings, or unquoted as a sequence of one or more identifiers. This means most punctuation characters and digits at the start of each token must be escaped in unquoted font family names.
To avoid mistakes in escaping, it is recommended to quote font family names that contain white space, digits, or punctuation characters other than hyphens
There was a problem hiding this comment.
This rule will stumble on unquoted multi-word font names and unquoted font names containing escape sequences. Wrap these font names in quotation marks, and everything should be fine.
How about ^^?
There was a problem hiding this comment.
Ok. Should I keep Caveat word?
There was a problem hiding this comment.
Yep, to match the format from other docs.
| } | ||
| else { | ||
| familyNames.add(family) | ||
| } |
There was a problem hiding this comment.
Ok, I guess I wasn't looking closely enough. I still think this code can be simplified, though. Please try to match the conventions in other parts of the codebase for brace spacing and early returns, and see if you can this up a little.
gaidarenko
force-pushed
the
issue-1284
branch
from
fbdfe1c
to
4b66482
Compare
|
I have simplified the code to use early returns. |
| } | ||
|
|
||
| keywords.add(family) | ||
| } |
There was a problem hiding this comment.
Can return early here instead of the else statement below.
gaidarenko
force-pushed
the
issue-1284
branch
from
4b66482
to
972c453
Compare
|
Done. |
|
👍 from me. @jeddy3 any input, or should we merge? |
|
@davidtheclark what about add option |
| Disallow duplicate font family names. | ||
|
|
||
| ```css | ||
| a { font-family: "Times New Roman", serif; } |
There was a problem hiding this comment.
a { font-family: serif, serif; }
/** ↑ ↑
* These font family names */| import { | ||
| messages, | ||
| ruleName, | ||
| } from ".." |
Two very minor requests. Otherwise, looks good to me. @gaidarenko Thanks for yet another rule! :)
This is what the disable comment are for i.e. one-off instances (e.g. hacks or workarounds). The |
|
@jeddy3 agree |
gaidarenko
force-pushed
the
issue-1284
branch
from
972c453
to
38c1103
Compare
|
Done. |
|
Closes #1284
e.g. "No, it's self explanatory."