Skip to content

Correct pill font#148

Merged
pixelbandito merged 3 commits intomasterfrom
correct-pill-font
Feb 18, 2020
Merged

Correct pill font#148
pixelbandito merged 3 commits intomasterfrom
correct-pill-font

Conversation

@kcj002
Copy link
Copy Markdown
Contributor

@kcj002 kcj002 commented Feb 16, 2020

Pill font styling was Source Sans Pro, which was TrendKite styling. Post rebrand, Source Sans Pro was replaced with Proxima Nova. Removed styling for Menu.Item and Pill so that global font of Proxima Nova would be used.

Risk: low, just a font update.

@pixelbandito
Copy link
Copy Markdown
Contributor

Hey, @kcj002 any reason not to put the new font in the variables.css instead?
Without a font-family variable, we'll be relying on the hosting app to handle that part of the style.

@kcj002
Copy link
Copy Markdown
Contributor Author

kcj002 commented Feb 17, 2020

I thought the font-family: 'Proxima Nova', sans-serif; for :root in colors.css would cover font assignment. Is that incorrect?

@pixelbandito
Copy link
Copy Markdown
Contributor

I thought the font-family: 'Proxima Nova', sans-serif; for :root in colors.css would cover font assignment. Is that incorrect?

Ah, I missed that because it's not supposed to be in colors.css. Would you mind moving it to variables.css as part of your change?

Comment thread src/shared/variables.css Outdated
@@ -1,10 +1,11 @@
:root {
font-family: 'Proxima Nova', sans-serif;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry I didn't realize that was a style rule in colors.css - I thought it was a CSS variable.
Safe to just omit it, but it shouldn't hurt anything.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So I can remove this from variables and colors.css and then recreate the font variable and use that for Menu.Item and Pill

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, exactly. If/when we want to apply a font-family to a component, we should re-add it to variables.css

If you want to do that now, great, but it can also wait if necessary.

@pixelbandito pixelbandito merged commit a8bf2e0 into master Feb 18, 2020
@pixelbandito pixelbandito deleted the correct-pill-font branch February 18, 2020 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants