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

Default to primitive CSS variable values for SASS typography line-height and size #2765

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

iansan5653
Copy link

@iansan5653 iansan5653 commented Jan 13, 2025

What are you trying to accomplish?

The typography SASS variables for font sizes align with the primitive CSS variables in value, but the default line-height differs. This results in inconsistent spacing in body text - for example, the Markdown editor uses the 1.4285 value while the markdown-body class uses 1.5, resulting in inconsistent spacing across the Markdown preview vs editor (as noted on Slack).

What approach did you choose and why?

To fix this, we can update the value for lh-default to match the primitive. To ensure future consistency I updated it to be the primitive. I also updated the small & default font sizes to use the primitives as well (this has no visual change). This reduces the line-height by 0.0715 (1.001 px).

Note that I did not update lh-condensed and lh-condensed-ultra since there are no existing primitives that match these values (that I know of).

What should reviewers focus on?

Can these changes ship as is?

  • Yes, this PR does not depend on additional changes. 🚢

@iansan5653 iansan5653 requested a review from a team as a code owner January 13, 2025 16:03
Copy link

changeset-bot bot commented Jan 13, 2025

🦋 Changeset detected

Latest commit: aa88388

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/css Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@iansan5653 iansan5653 changed the title Default to CSS variable values for typography Default to primitive CSS variable values for SASS typography line-height and size Jan 13, 2025
@github-actions github-actions bot temporarily deployed to Storybook Preview January 13, 2025 16:06 Inactive
Copy link
Contributor

@langermank langermank left a comment

Choose a reason for hiding this comment

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

This change scares me a bit 😆 I think changing that default line-height could lead to visual changes which could be both good and bad depending on the context. What if we just update the CSS specific to markdown to isolate the fix? Not a global/long term fix really but these older files typically need a lot of testing to change.

@iansan5653
Copy link
Author

Would it be enough to deploy this to lab and test it out? It is a very slight change (1 pixel) and it seems generally good to align on one line height and discourage inconsistency more broadly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants