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

fix(charts): use vars to enable Red Hat fonts #2584

Merged
merged 1 commit into from Jul 24, 2019

Conversation

@dlabrecq
Copy link
Member

dlabrecq commented Jul 24, 2019

When the pf-m-redhat-font selector is added to the page, the Red Hat font is expected to be used instead of the default Overpass font.

This change ensures that charts and its labels use the Red Hat font as expected.

Fixes #2573

Screen Shot 2019-07-24 at 9 50 40 AM

@tlabaj tlabaj added the css review label Jul 24, 2019
@tlabaj tlabaj requested a review from mcoker Jul 24, 2019
@tlabaj tlabaj added the PF4 label Jul 24, 2019
@@ -7,7 +7,9 @@ exports[`Chart 1`] = `
lineHeight={1}
style={
Object {
"fontFamily": "overpass, overpass, open sans, -apple-system, blinkmacsystemfont, Segoe UI, roboto, Helvetica Neue, arial, sans-serif, Apple Color Emoji, Segoe UI Emoji, Segoe UI Symbol",
"fontFamily": "var(--pf-chart-global--FontFamily)",
"fontSize": 14,

This comment has been minimized.

Copy link
@mcoker

mcoker Jul 24, 2019

Contributor

we have vars for the font sizes used in charts - this one is --pf-chart-global--FontSize--sm

https://github.com/patternfly/patternfly-next/blob/master/src/patternfly/_chart-globals.scss#L55-L61

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Jul 24, 2019

Author Member

Victory typically requires raw values for colors, pixels, etc. The font size; for example, must be output as a value because Victory concatenates that with 'px'.

Although the snapshot does not reflect that. The font family and letter spacing are one of the few places where we can output a variable.

This comment has been minimized.

Copy link
@mcoker

mcoker Jul 24, 2019

Contributor

Should we remove the unit from these vars so they're usable? We do that with other properties like padding, margin, width, height, etc.

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Jul 24, 2019

Author Member

There is no unit in our pf-core chart vars for this very reason, it's just a raw number.

Victory is appending the unit automatically. I suspect because they use the raw value to calculate heights & widths of labels, legends, etc. Thus, a CSS variable breaks for properties like fontSize, padding, etc.

Although users can create a custom theme, I've asked Victory to better support CSS vars.
https://github.com/FormidableLabs/victory/issues/1357

@@ -22,7 +24,9 @@ exports[`Chart 2`] = `
lineHeight={1}
style={
Object {
"fontFamily": "overpass, overpass, open sans, -apple-system, blinkmacsystemfont, Segoe UI, roboto, Helvetica Neue, arial, sans-serif, Apple Color Emoji, Segoe UI Emoji, Segoe UI Symbol",
"fontFamily": "var(--pf-chart-global--FontFamily)",
"fontSize": 14,

This comment has been minimized.

Copy link
@mcoker

mcoker Jul 24, 2019

Contributor

same here

@@ -37,7 +41,9 @@ exports[`renders component text 1`] = `
lineHeight={1}
style={
Object {
"fontFamily": "overpass, overpass, open sans, -apple-system, blinkmacsystemfont, Segoe UI, roboto, Helvetica Neue, arial, sans-serif, Apple Color Emoji, Segoe UI Emoji, Segoe UI Symbol",
"fontFamily": "var(--pf-chart-global--FontFamily)",
"fontSize": 14,

This comment has been minimized.

Copy link
@mcoker

mcoker Jul 24, 2019

Contributor

same here

@dlabrecq dlabrecq force-pushed the dlabrecq:2573-chart-fonts branch from 12420d7 to 88acf76 Jul 24, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jul 24, 2019

PatternFly-React preview: https://patternfly-react-pr-2584.surge.sh

@mcoker
mcoker approved these changes Jul 24, 2019
Copy link
Contributor

mcoker left a comment

👍

@tlabaj
tlabaj approved these changes Jul 24, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit d555f2a into patternfly:master Jul 24, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@dlabrecq dlabrecq deleted the dlabrecq:2573-chart-fonts branch Jul 25, 2019
@dlabrecq dlabrecq changed the title fix(charts): use vars to enable Red Hat fonts [ci skip] fix(charts): use vars to enable Red Hat fonts Jul 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.