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

chore: removing unneeded typography classnames from the tailwind preset #117

Merged
merged 4 commits into from
Nov 15, 2024
Merged

chore: removing unneeded typography classnames from the tailwind preset #117

merged 4 commits into from
Nov 15, 2024

Conversation

georgewrmarshall
Copy link
Contributor

@georgewrmarshall georgewrmarshall commented Nov 15, 2024 •
edited
Loading

Description

This PR removes unused font weight variants from the Tailwind preset classname generation. Previously, font weights such as body-md-medium and body-md-bold were included, where medium and bold specified font weights. With this update, we no longer use these variants. Instead, font weight is now managed through the fontWeight prop, aligning with recent deprecations aimed at simplifying the styling logic. This change reduces confusion and improves maintainability by eliminating redundant classnames.

Related issues

Fixes: N/A (Part of Text component)

Manual testing steps

  1. Look at the packages/design-system-tailwind-preset/src/typography.ts file
  2. Ensure there are no remaining font weight suffixes defined in the tailwind preset configuration
  3. Ensure tests and build run as expected

Screenshots/ Screen Recordings

Before

Recording showing incorrect medium weight BodyLg variant

before720.mov

Recording showing many suffixed classnames with font weights

before72.mov

After

Recording showing fixed BodyLg variant

after720.mov

Recording showing only suffixes that remain are CSS variables and tests

after720.mov

Pre-merge author checklist

  • I've followed MetaMask Contributor Docs.
  • I've completed the PR template to the best of my ability.
  • I’ve included tests if applicable.
  • I’ve documented my code using JSDoc format if applicable.
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

  • I’ve manually tested the PR (e.g., pull and build branch, run the app, test code changes).
  • I confirm that this PR addresses all acceptance criteria and includes necessary testing evidence, such as recordings or screenshots.

@georgewrmarshall georgewrmarshall self-assigned this Nov 15, 2024
@georgewrmarshall georgewrmarshall marked this pull request as ready for review November 15, 2024 17:40
@georgewrmarshall georgewrmarshall requested a review from a team as a code owner November 15, 2024 17:40
georgewrmarshall
georgewrmarshall commented Nov 15, 2024
View reviewed changes
packages/design-system-tailwind-preset/src/typography.test.ts
@@ -20,6 +20,61 @@ describe('Typography', () => {
'--typography-l-heading-sm-regular-line-height',
'--typography-l-heading-sm-regular-font-weight',
'--typography-l-heading-sm-regular-letter-spacing',
'--typography-s-body-lg-regular-font-family',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added all the removed variables to the ignore list. We have a test in place to ensure all typography variables are used. In a future design-tokens version, these variables will be removed as they are now deprecated.

georgewrmarshall
georgewrmarshall commented Nov 15, 2024
View reviewed changes
packages/design-system-react/src/components/text/Text.tsx
@@ -33,7 +33,7 @@ const typographyClassMap: Record<TextVariant, string> = {
[TextVariant.HeadingSm]:
'text-s-heading-sm font-s-heading-sm leading-s-heading-sm tracking-s-heading-sm lg:text-l-heading-sm lg:font-l-heading-sm lg:leading-l-heading-sm lg:tracking-l-heading-sm',
[TextVariant.BodyLg]:
'text-s-body-lg-medium font-s-body-lg-medium leading-s-body-lg-medium tracking-s-body-lg-medium lg:text-l-body-lg-medium lg:font-l-body-lg-medium lg:leading-l-body-lg-medium lg:tracking-l-body-lg-medium',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing font weight prefix from Text component

georgewrmarshall
georgewrmarshall commented Nov 15, 2024
View reviewed changes
packages/design-system-tailwind-preset/src/typography.ts
@@ -5,29 +5,18 @@ export const typography = {
's-heading-lg': 'var(--typography-s-heading-lg-font-size)',
Copy link
Contributor Author

@georgewrmarshall georgewrmarshall Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In this file we are removing all typography variant classnames with a font-weight suffix. e.g. -regular, -medium, -bold.

These should be the only suffixes that exist in this file

Screenshot 2024-11-15 at 11 25 06 AM
Screenshot 2024-11-15 at 11 25 13 AM
Screenshot 2024-11-15 at 11 25 24 AM

georgewrmarshall
georgewrmarshall commented Nov 15, 2024
View reviewed changes
packages/design-system-tailwind-preset/src/typography.ts
's-body-lg-regular': 'var(--typography-s-body-lg-regular-font-size)',
's-body-md-bold': 'var(--typography-s-body-md-bold-font-size)',
's-body-md-medium': 'var(--typography-s-body-md-medium-font-size)',
's-body-lg': 'var(--typography-s-body-lg-regular-font-size)',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately our Token Studio had to generate CSS variables that mirrored the Figma styles so we don't have any css variables without the font weight prefix

georgewrmarshall
georgewrmarshall commented Nov 15, 2024
View reviewed changes
packages/design-system-tailwind-preset/src/typography.ts
'l-body-lg-medium': 'var(--typography-l-body-lg-medium-font-weight)',
'l-body-md-bold': 'var(--typography-l-body-md-bold-font-weight)',
'l-body-md-medium': 'var(--typography-l-body-md-medium-font-weight)',
'l-body-lg': 'var(--font-weight-regular)',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out we forgot to create the CSS variables for large screen so falling back to the font weight token here

georgewrmarshall
georgewrmarshall commented Nov 15, 2024
View reviewed changes
packages/design-system-tailwind-preset/src/typography.ts
's-body-lg-regular': 'var(--typography-s-body-lg-regular-font-weight)',
's-body-md-bold': 'var(--typography-s-body-md-bold-font-weight)',
's-body-md-medium': 'var(--typography-s-body-md-medium-font-weight)',
's-body-lg': 'var(--font-weight-regular)',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

--typography-l-body-lg-regular-letter-spacing CSS token incorrectly points to medium? so falling back to root font weight token here too

https://github.com/MetaMask/design-tokens/blob/main/src/css/typography.css#L73

@georgewrmarshall georgewrmarshall mentioned this pull request Nov 15, 2024
Open
7 tasks
@georgewrmarshall georgewrmarshall merged commit 891c8a1 into main Nov 15, 2024
26 checks passed
@georgewrmarshall georgewrmarshall deleted the fix/preset-package-updates branch November 15, 2024 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brianacnguyen brianacnguyen brianacnguyen approved these changes

Assignees

@georgewrmarshall georgewrmarshall

Labels
team-design-system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@georgewrmarshall @brianacnguyen