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.

Sign up for GitHub

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

Jump to bottom

feat(font): add option to use new red hat font #1813

Merged
merged 2 commits into from
Jul 12, 2019
Merged

feat(font): add option to use new red hat font #1813

merged 2 commits into from
Jul 12, 2019

Conversation

mcoker
Copy link
Contributor

@mcoker mcoker commented May 10, 2019 •
edited
Loading

fixes #1524

@mcoker mcoker requested review from kybaker and mceledonia May 10, 2019 20:40
@patternfly-build
Copy link

patternfly-build commented May 10, 2019 •
edited
Loading

Deploy preview for pf-next ready!

Built with commit dbd378c

https://deploy-preview-1813--pf-next.netlify.com

@christiemolloy
Copy link
Member

I know this is a WIP, but noticed that the h4 here looks too bold, because it wasn't for display purposes but rather screen readers.

Screen Shot 2019-05-13 at 2 08 42 PM

@christiemolloy christiemolloy mentioned this pull request May 13, 2019
Closed
@mcoker
Copy link
Contributor Author

mcoker commented May 13, 2019

@christiemolloy yep, all of the <h1-6> elements now use the "display" font, and it's "normal" font-weight is bolder than the text version of the new font. All of this may change so that we're, for example, only applying the "display" font to .pf-c-content h1-6 and/or .pf-c-title, and leaving random uses of <h1-6> as the "text" font, but I'm not sure. Want to go through it with @kybaker and see what he expects.

@christiemolloy
Copy link
Member

@mcoker great yeah I saw your comment above, I think that option would work better.

@mcoker mcoker force-pushed the issue-1524 branch 2 times, most recently from 3772cc1 to ca422d2 Compare June 21, 2019 22:57
christiemolloy
christiemolloy approved these changes Jul 12, 2019
View reviewed changes
Copy link
Member

@christiemolloy christiemolloy left a comment

Choose a reason for hiding this comment

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

Looks great to me!

mattnolting
mattnolting approved these changes Jul 12, 2019
View reviewed changes
Copy link
Contributor

@mattnolting mattnolting left a comment

Choose a reason for hiding this comment

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

Beautiful!

@dgutride dgutride merged commit 3eceded into patternfly:master Jul 12, 2019
@patternfly-build
Copy link

🎉 This PR is included in version 2.18.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mcoker mcoker mentioned this pull request Jul 30, 2019
Closed
@rhamilto
Copy link
Member

rhamilto commented Sep 6, 2019

With this implementation, two sets of font files are shipping regardless of which one is being used. This seems less than ideal. Any reason why the SASS doesn't only include the relevant fonts based upon a variable?

cc: @spadgett

@mcoker
Copy link
Contributor Author

mcoker commented Sep 6, 2019

Hey @rhamilto! We discussed this approach and since we have a lot of users that don't use SCSS and import CSS directly, we wanted to have a single approach for everyone. Firefox, Chrome, and Safari only download fonts that are used on the page, so those browsers shouldn't download Overpass, even if it's included in the CSS.

That said, if this approach doesn't work for you, I'm happy to offer an alternate solution for you or anyone else that might have these same concerns.

@rhamilto
Copy link
Member

rhamilto commented Sep 6, 2019

The issue is not the browsers downloading the fonts, but the fonts being distributed with the app. We're shipping an extra 500 kb of fonts our app doesn't use!

@rhamilto
Copy link
Member

rhamilto commented Sep 6, 2019

Seems like there should be an optional SASS variable available to switch the inclusion of the correct fonts.

@mcoker mcoker mentioned this pull request Sep 6, 2019
Closed
@dgutride dgutride mentioned this pull request Sep 10, 2019
Closed
@redallen
Copy link
Contributor

Added a wiki page on how to achieve what @rhamilto wants: https://github.com/patternfly/patternfly-next/wiki/Overpass-vs-Red-Hat-Font

@mcoker mcoker deleted the issue-1524 branch December 16, 2019 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christiemolloy christiemolloy christiemolloy approved these changes

@mattnolting mattnolting mattnolting approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mcoker @patternfly-build @christiemolloy @rhamilto @redallen @mattnolting @dgutride