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

Removes serif from Text #734

Merged
merged 4 commits into from
Jul 20, 2020
Merged

Removes serif from Text #734

merged 4 commits into from
Jul 20, 2020

Conversation

dzucconi
Copy link
Member

Also adds the overflowEllipsis option, and updates the typography docs. I would have added a bit more to those but I can't get the docs to actually compile correctly.

@dzucconi dzucconi requested a review from damassi July 20, 2020 16:00
@dzucconi dzucconi changed the title Removes serif from typography Removes serif from Text Jul 20, 2020

@media (min-width: ${themeGet("breakpoints.0")}) {
${variant({ variants: TEXT_VARIANTS.large })}
${textMixin}
Copy link
Member Author

Choose a reason for hiding this comment

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

Just PR-creeped this bug fix in. Due to how styled-components emits the media query CSS, we need to double include the textMixin in both contexts so when you specify say a solid line-height for Text (which is what I need in the NavBar, for instance — a valid use-case, the media query doesn't override it.

@damassi
Copy link
Member

damassi commented Jul 20, 2020

LGTM 👍

#mergeongreen

@dzucconi dzucconi force-pushed the remove-serif-add-ellipsis branch from 39a507a to 510f1c0 Compare July 20, 2020 16:56
@artsy-peril artsy-peril bot merged commit 4fb8629 into master Jul 20, 2020
@artsyit
Copy link
Contributor

artsyit commented Jul 20, 2020

🚀 PR was released in v11.10.0 🚀

@damassi damassi deleted the remove-serif-add-ellipsis branch July 20, 2020 21:39
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.

3 participants