8000 fix(market-sans): fix line-height for new font version by ArtBlue · Pull Request #2586 · eBay/skin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Apr 28, 2025. It is now read-only.

fix(market-sans): fix line-height for new font version #2586

Merged
merged 3 commits into from
Feb 28, 2025
Merged

fix(market-sans): fix line-height for new font version #2586

merged 3 commits into from
Feb 28, 2025

Conversation

ArtBlue
Copy link
Contributor
@ArtBlue ArtBlue commented Feb 27, 2025

Fixes #2583

  • This PR contains CSS changes
  • This PR does not contain CSS changes

Description

Fixed framework-wide vertical font spacing issues introduced by the Market Sans v2 upgrade by adding explicit scalable line-height.

Notes

W're going to be using unitless line-height. The key difference is that 1.42 is unitless and directly scales with the font size, while 1.42em is explicitly tied to the font size unit. In most cases, for line-height, using a unitless value is preferred because it scales better with different font sizes and ensures consistent vertical rhythm.

Screenshots

Before

image

After

image

Checklist

  • I verify the build is in a non-broken state
  • I verify all changes are within scope of the linked issue
  • I verify the markup will not be a breaking change (if not a major release)
  • I verify the MIND pattern for the component has been created/revised
  • I regenerated all CSS files under dist folder
  • I tested the UI in all supported browsers
  • I did a visual regression check of the components impacted by doing a Percy build and approved the build
  • I tested the UI in dark mode and RTL mode
  • I added/updated/removed Storybook coverage as appropriate

@ArtBlue ArtBlue self-assigned this Feb 27, 2025
@ArtBlue ArtBlue linked an issue Feb 27, 2025 that may be closed by this pull request
market-sans: fix font spacing issues across components #2583
Closed
@changeset-bot changeset-bot
Copy link
changeset-bot bot commented Feb 27, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: d64f201

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

This PR includes changesets to release 1 package
Name Type
@ebay/skin Patch

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

LuLaValva
LuLaValva previously approved these changes Feb 27, 2025
View reviewed changes
agliga
agliga reviewed Feb 27, 2025
View reviewed changes
Copy link
Contributor
@agliga agliga left a comment

Choose a reason for hiding this comment

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

I see some changes in spacing on unrelated files. Can you reinstall npm packages and rerun build.
I had changed them recently because of prettier

@ArtBlue
Copy link
Contributor Author
ArtBlue commented Feb 28, 2025

I see some changes in spacing on unrelated files. Can you reinstall npm packages and rerun build. I had changed them recently because of prettier

Done! Good catch!

agliga
agliga approved these changes Feb 28, 2025
View reviewed changes
Copy link
Contributor
@agliga agliga left a comment

Choose a reason for hiding this comment

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

Lgtm!

@ArtBlue ArtBlue merged commit 3dbaf75 into master Feb 28, 2025
@github-actions github-actions bot mentioned this pull request Feb 28, 2025
Merged
@agliga agliga deleted the 2583-line-heights branch March 31, 2025 19:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@agliga agliga agliga approved these changes

@LuLaValva LuLaValva LuLaValva left review comments

@saiponnada saiponnada Awaiting requested review from saiponnada

Assignees

@ArtBlue ArtBlue

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

market-sans: fix font spacing issues across components
3 participants
@ArtBlue @agliga @LuLaValva
0