FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(charts): use vars to enable Red Hat fonts by dlabrecq · Pull Request #2584 · patternfly/patternfly-react · GitHub

/ patternfly-react Public

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

Merged
tlabaj merged 1 commit into
patternfly:masterfrom
dlabrecq:2573-chart-fonts
Jul 24, 2019
ConversationCommits (1)ChecksFiles changed
Merged

fix(charts): use vars to enable Red Hat fonts#2584
tlabaj merged 1 commit into
patternfly:masterfrom
dlabrecq:2573-chart-fonts

Conversation

dlabrecq commented Jul 24, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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

tlabaj requested a review from mcoker July 24, 2019 14:09
tlabaj added the PF4 label Jul 24, 2019
mcoker reviewed Jul 24, 2019
View reviewed changes
Comment thread ...s/patternfly-4/react-charts/src/components/ChartLabel/__snapshots__/ChartLabel.test.tsx.snap
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,

mcoker Jul 24, 2019

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

dlabrecq Jul 24, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

mcoker Jul 24, 2019

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

dlabrecq Jul 24, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mcoker reviewed Jul 24, 2019
View reviewed changes
Comment thread ...s/patternfly-4/react-charts/src/components/ChartLabel/__snapshots__/ChartLabel.test.tsx.snap
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,

mcoker Jul 24, 2019

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

same here

mcoker reviewed Jul 24, 2019
View reviewed changes
Comment thread ...s/patternfly-4/react-charts/src/components/ChartLabel/__snapshots__/ChartLabel.test.tsx.snap
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,

mcoker Jul 24, 2019

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

same here

patternfly-build commented Jul 24, 2019

Copy link
Copy Markdown
Collaborator

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

mcoker approved these changes Jul 24, 2019
View reviewed changes

mcoker left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

👍

tlabaj approved these changes Jul 24, 2019
View reviewed changes

tlabaj left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

tlabaj merged commit d555f2a into patternfly:master Jul 24, 2019
dlabrecq deleted the 2573-chart-fonts branch July 25, 2019 14:54
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcoker mcoker approved these changes
+2 more reviewers
tlabaj tlabaj approved these changes
redallen redallen approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update charts to use Red Hat font

5 participants

Back | FazBrowse Home | New Git URL