8000 chore(fonts): update font names and remove extra tokens by srambach · Pull Request #6857 · patternfly/patternfly · GitHub
[go: up one dir, main page]
Skip to content

chore(fonts): update font names and remove extra tokens#6857

Merged
mcoker merged 2 commits intopatternfly:v6from
srambach:6790-update-fonts
Jul 9, 2024
Merged

chore(fonts): update font names and remove extra tokens#6857
mcoker merged 2 commits intopatternfly:v6from
srambach:6790-update-fonts

Conversation

@srambach
Copy link
Member
@srambach srambach commented Jul 7, 2024

Fixes #6790

Updates font names to match tokens (include spaces)
Also removes font tokens from the local token file since they are now in the default token file from Figma.

Note: There are two local tokens for font weight that are used but not in Figma - --pf-t--global--font--weight--body--bold and --pf-t--global--font--weight--heading--bold. These point to base tokens that also don't exist. (see diff for details) @lboehling should these semantic font weight tokens exist and should they point to existing base font weigh tokens?

@patternfly-build
Copy link
Collaborator
patternfly-build commented Jul 7, 2024 •
edited
Loading

Preview: https://patternfly-pr-6857.surge.sh

A11y report: https://patternfly-pr-6857-a11y.surge.sh

@srambach srambach linked an issue Jul 8, 2024 that may be closed by this pull request
Update @font-face font-family to match token format from figma #6790
Closed
mcoker
mcoker reviewed Jul 8, 2024
View reviewed changes
src/patternfly/base/tokens/tokens-local.scss
// Manually added from hackathon values
// Currently not being exported from Figma because they are styles rather than variables
// The ability to make font variables is reportedly coming by the end of the year
// LOCAL TOKENS
Copy link
Contributor
@mcoker mcoker Jul 8, 2024

Choose a reason for hiding this comment

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

🥸👍

mcoker
mcoker reviewed Jul 9, 2024
View reviewed changes
Copy link
Contributor
@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

Left a couple of comments about using the font-weight tokens from design, though looks like we're missing the --bold tokens from design. We could either merge the PR as is or apply my suggestions - then follow up with design to add the --bold tokens, then remove our local font-weight tokens.

Also looks like we can yank these text-decoration tokens out, too. We have a whole bushel of 'em now! Lemme know if you'd like to do that with this PR or as a follow-up.

patternfly/src/patternfly/base/tokens/tokens-local.scss

Lines 31 to 36 in 42dfd82

// Other missing ones
// text-decoration
--pf-t--global--text-decoration--100: none;
--pf-t--global--text-decoration--200: underline;
--pf-t--global--link--text-decoration: var(--pf-t--global--text-decoration--100);
--pf-t--global--link--text-decoration--hover: var(--pf-t--global--text-decoration--200);

patternfly/src/patternfly/base/tokens/tokens-default.scss

Lines 293 to 304 in 42dfd82

--pf-t--global--text-decoration--editable-text--line--default: var(--pf-t--global--text-decoration--line--200);
--pf-t--global--text-decoration--editable-text--line--hover: var(--pf-t--global--text-decoration--line--200);
--pf-t--global--text-decoration--editable-text--style--default: var(--pf-t--global--text-decoration--style--200);
--pf-t--global--text-decoration--editable-text--style--hover: var(--pf-t--global--text-decoration--style--200);
--pf-t--global--text-decoration--help-text--line--default: var(--pf-t--global--text-decoration--line--200);
--pf-t--global--text-decoration--help-text--line--hover: var(--pf-t--global--text-decoration--line--200);
--pf-t--global--text-decoration--help-text--style--default: var(--pf-t--global--text-decoration--style--200);
--pf-t--global--text-decoration--help-text--style--hover: var(--pf-t--global--text-decoration--style--200);
--pf-t--global--text-decoration--link--line--default: var(--pf-t--global--text-decoration--style--200);
--pf-t--global--text-decoration--link--line--hover: var(--pf-t--global--text-decoration--style--200);
--pf-t--global--text-decoration--link--style--default: var(--pf-t--global--text-decoration--style--100);
--pf-t--global--text-decoration--link--style--hover: var(--pf-t--global--text-decoration--style--100);

src/patternfly/base/tokens/tokens-local.scss Outdated
Comment on lines 5 to 8
--pf-t--global--font--weight--body--100: 400; // not currently used
--pf-t--global--font--weight--body--200: 500;
--pf-t--global--font--weight--heading--100: 700;
--pf-t--global--font--weight--heading--100: 700; // not currently used
--pf-t--global--font--weight--heading--200: 700;
Copy link
Contributor
@mcoker mcoker Jul 8, 2024

Choose a reason for hiding this comment

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

I think we can yank out these font-weight tokens, too. We have this now:

patternfly/src/patternfly/base/tokens/tokens-default.scss

Lines 71 to 74 in 42dfd82

--pf-t--global--font--weight--100: 400;
--pf-t--global--font--weight--200: 500;
--pf-t--global--font--weight--300: 700;
--pf-t--global--font--weight--400: 700;

src/patternfly/base/tokens/tokens-local.scss Outdated
--pf-t--global--font--line-height--heading: var(--pf-t--global--font--line-height--100);
--pf-t--global--font--weight--body: var(--pf-t--global--font--weight--body--100);
// Missing semantic font tokens
--pf-t--global--font--weight--body--bold: var(--pf-t--global--font--weight--body--200);
Copy link
Contributor
@mcoker mcoker Jul 8, 2024

Choose a reason for hiding this comment

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

If we use the font-weights from figma, we can update this

Suggested change
--pf-t--global--font--weight--body--bold: var(--pf-t--global--font--weight--body--200);
--pf-t--global--font--weight--body--bold: var(--pf-t--global--font--weight--200);

src/patternfly/base/tokens/tokens-local.scss Outdated
// Missing semantic font tokens
--pf-t--global--font--weight--body--bold: var(--pf-t--global--font--weight--body--200);
--pf-t--global--font--weight--heading: var(--pf-t--global--font--weight--heading--100);
--pf-t--global--font--weight--heading--bold: var(--pf-t--global--font--weight--heading--200);
Copy link
Contributor
@mcoker mcoker Jul 9, 2024

Choose a reason for hiding this comment

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

And if we use the font-weights from figma, we can update this, too.

Suggested change
--pf-t--global--font--weight--heading--bold: var(--pf-t--global--font--weight--heading--200);
--pf-t--global--font--weight--heading--bold: var(--pf-t--global--font--weight--400);

@lboehling
Copy link
lboehling commented Jul 9, 2024 •
edited
Loading

@srambach @mcoker the bold weight tokens are in figma, I wonder why they're not exporting?
image

@srambach
Copy link
Member Author
srambach commented Jul 9, 2024

@srambach @mcoker the bold weight tokens are in figma, I wonder why they're not exporting? image

Ah...I think it's because there's a value for font--weight--body but then --bold is nested inside. I don't think the parser picks that up. I think that's one reason we added --default to most things.

Here's how bold is getting nested inside the body/heading

      "weight": {
        "body": {
          "description": "Use to define the default weight for body text",
          "type": "number",
          "value": "{global.font.weight.100}",
          "bold": {
            "description": "Use to define the bold weight for body text, often used to field labels or to add emphasis.",
            "type": "number",
            "value": "{global.font.weight.200}"
          }
        },
        "heading": {
          "description": "Use to define the default weight for heading text",
          "type": "number",
          "value": "{global.font.weight.300}",
          "bold": {
            "description": "Use to define the bold weight for heading text, often used to add emphasis.",
            "type": "number",
            "value": "{global.font.weight.400}"
          }
        }
      },

IDK if @evwilkin has any knowledge of making the dictionary handle that? Or we add --default I guess.

@srambach
Copy link
Member Author
srambach commented Jul 9, 2024

@mcoker I reassigned those missing bold tokens to the correct base tokens. However, those and the text-decoration tokens are still used in the code instead of the newer ones coming from figma. I'd like to do that as a followup.

@mcoker mcoker merged commit 1fca4ef into patternfly:v6 Jul 9, 2024
@patternfly-build
Copy link
Collaborator
patternfly-build commented Jul 9, 2024

🎉 This PR is included in version 6.0.0-alpha.185 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mcoker mcoker mentioned this pull request Jul 11, 2024
Closed
@patternfly-build patternfly-build mentioned this pull request Apr 16, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcoker mcoker mcoker approved these changes

@mattnolting mattnolting Awaiting requested review from mattnolting

@lboehling lboehling Awaiting requested review from lboehling

Assignees

No one assigned

Labels

released on @alpha

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update @font-face font-family to match token format from figma

4 participants

@srambach @patternfly-build @lboehling @mcoker
0