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

Generic font family check #334

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Olegas
Copy link

@Olegas Olegas commented Jan 9, 2013

New rule 'generic-font-family'

This rule checks, that each font/font-family declaration contains a 'generic' font family (one of the following: serif, sans-serif, cursive, monospace, fantasy).

Also, it is checks that 'generic' font family is specified as an identifier, not as a string, cause this prevents correct rendering.

Specifying a 'generic' font family is needed to keep text rendering as near as possible to desired style on systems, where other specified fonts are not exists, some kind of a fallback.

@Olegas
Copy link
Author

Olegas commented Jan 9, 2013

It is like #275 but has some more checks and avoiding some rough edges, which, as far as i can see, will fail in aforementioned task.

@nzakas
Copy link
Contributor

nzakas commented Jan 17, 2013

Can you provide some examples that this rule would catch?

@stubbornella thoughts?

@Olegas
Copy link
Author

Olegas commented Jan 18, 2013

Rule checks for font family declarations with no generic alternative

/* Warning! No generic! */
.clazz {
  font-family: Tahoma;
}

/* Warning! No generic! */
.clazz2 {
  font: 12px Georgia;
}

This is how we can fix above mentioned rules...

/* OK */
.clazz {
  font-family: Tahoma, sans-serif;
}

/* OK */
.clazz2 {
  font: 12px Georgia, serif;
}

Following lines will not cause a warning

/* OK */
@font-face {
    font-family: Pompadur; /* a custom name declaration */
    src: url(fonts/pompadur.ttf); /* a custom font file */
}

But the following usage - does

/* Warning! No generic! */
.myFont {
  font-family: Pompadur;
}

And how to fix it...

/* OK */
.myFont {
  font-family: Pompadur, sans-serif;
}

Also, using an inherit keyword will not flag

/* OK */
.child {
  font-family: inherit;
}

The another important thing is about quotes. "serif" and serif - this is not the same!

/* Warning! Quoted Generic */
.iLikeQuotes {
  font-family: Tahoma, "serif";
}

To fix it - just remove the quotes

/* OK */
.iLikeQuotes {
  font-family: Tahoma, serif;
}

@philipwalton
Copy link
Contributor

I mentioned this in another thread about this issue, but fallback fonts aren't needed for pseudo elements.

For example, if I define the following rule:

.icon-remove:before {
  content: "...";
  font-family: "MyIconFont";
}

This would be fine. If the font isn't found or there was an error in loading, it would fall back to the font defined on the parent rather than falling back to the system font.

@Olegas
Copy link
Author

Olegas commented Jan 18, 2013

@philipwalton sounds reasonable... I'll fix it and update the pull request.

@philipwalton
Copy link
Contributor

@Olegas I remember testing it with :before and :after, but you might want to check :first-letter and :first-line as well, just in case.

@Olegas
Copy link
Author

Olegas commented Jan 19, 2013

@philipwalton What do you think, how to deal with such rules:

.clazz,
p:hover {
  font-family: Tahoma;
}

.clazz lacks of generic, but p:hover doesn't...

@philipwalton
Copy link
Contributor

I'd say require a backup, since .clazz should have one. It's certainly not bad to have a backup font declared on a pseudo element, it just shouldn't be considered an error if it's not there.

@frvge
Copy link
Contributor

frvge commented Jan 10, 2016

@Olegas , could you please give fixing @philipwalton 's comments a try?

@Olegas
Copy link
Author

Olegas commented Jan 15, 2016

Will take a look soon, this weekend. Sorry for delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants