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 generic font families #68

Closed
wants to merge 1 commit into from
Closed

Conversation

Unisay
Copy link
Contributor

@Unisay Unisay commented Aug 12, 2017

As per https://developer.mozilla.org/en/docs/Web/CSS/font-family

  • serif
  • sans-serif
  • monospace
  • cursive
  • fantasy
  • system-ui

@CLAassistant
Copy link

CLAassistant commented Aug 12, 2017

CLA assistant check
All committers have signed the CLA.

@thomashoneyman thomashoneyman changed the base branch from master to main October 6, 2020 03:09
Copy link
Contributor

@JordanMartinez JordanMartinez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine to add. We already have sansSerif and this isn't a core library. If we're going to support these, perhaps we should support all the ones mentioned in the link above?

@thomashoneyman
Copy link
Contributor

Yes, I think we should support the full set of generic font families as listed in the MDN docs.

@thomashoneyman
Copy link
Contributor

I’m a little surprised that it’s not a sum type, since they’re an enumeration, but perhaps it’s to avoid breaking changes if more are added in the future? I would be happy changing to a sum type also.

@JordanMartinez
Copy link
Contributor

JordanMartinez commented Sep 20, 2021

The downside to adding a sum type is that it would be a breaking change every time a new font family is added. Since this is likely just being used to ensure the string value (e.g. "monospace") is spelled correctly, I'm not sure we want to do that.

@thomashoneyman
Copy link
Contributor

I suppose I don’t know how often these change (CSS properties tend to be stable). Typically for things that don’t change often and which are enumerations I’d reach for a sum type but I’m also happy with a newtype here. I still think we should add the remaining cases, though.

@JordanMartinez
Copy link
Contributor

Superceded by #136

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

Successfully merging this pull request may close these issues.

None yet

4 participants