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

Add support for different units in TextStyle.lineHeight #156

Merged

Conversation

blinpete
Copy link
Contributor

@blinpete blinpete commented Feb 3, 2024

This pull request adds support for different line height units.

Issue

Currently TextStyle.lineHeight is a unitless number and stores lineHeightPx coming from figma-js and Figma API itself. Which results in unexpected behavior when you set up your line height in font size percentage but still end up with pixels exported.

Having this as the default behavior is a bit confusing cuz a unitless line height in web development carries a different meaning.

Solution

Figma supports 3 line height units PIXELS, INTRINSIC_% and FONT_SIZE_%.
Now TextStyle.lineHeight is a string and the textStyle parser handles all of the units correctly according to lineHeightUnit value.

The difference in behavior

{
  type: "TEXT",
  style: {
      ...,
      lineHeightPx: 30,
      lineHeightPercent: 106,
      lineHeightPercentFontSize: 125,
      lineHeightUnit: ... // "PIXELS" or "INTRINSIC_%" or "FONT_SIZE_%"
  }
}

Considering this Figma TextNode here's how figma-export handles a line height depending on the lineHeightUnit key:

Figma API lineHeightUnit figma-export (main branch) figma-export (on this branch)
PIXELS lineHeight: 30 lineHeight: "30px"
INTRINSIC_% lineHeight: 30 lineHeight: "106%"
FONT_SIZE_% lineHeight: 30 lineHeight: "1.25"

@marcomontalbano Thanks for your work, figma-export is damn well designed!

@marcomontalbano
Copy link
Owner

Hi @blinpete, thanks a lot for your PR, it looks amazing!

I have just one doubt. In my figma-export example file, I have two different texts (different font-size) with line-height set to Auto.

// first text with line-height: Auto
{
  type: "TEXT",
  style: {
    ...,
    "fontSize": 14,
    "lineHeightPx": 16.40625,
    "lineHeightPercent": 100,
    "lineHeightUnit": "INTRINSIC_%"
  }
}

// second text with line-height: Auto
{
  type: "TEXT",
  style: {
    ...,
    "fontSize": 18,
    "lineHeightPx": 21.09375,
    "lineHeightPercent": 100,
    "lineHeightUnit": "INTRINSIC_%"
  }
}

Before this change, the line height was generated as 16.40625px and 21.09375px respectively. With this PR it will change to 100% for both and it will render differently from before (I tested here for the final result and it looks like a breaking change).

What do you think about it? How can we manage it?

@blinpete
Copy link
Contributor Author

blinpete commented Feb 7, 2024

Hey @marcomontalbano!

not sure about this

  1. we can try to dive into typography and calculate what Figma is doing under the hood. There are tricks like basekick and capsizecss. seems like SEEK-OSS are far ahead here.
  2. or we can revert INTRINSIC_% if-branch to use pixels. INTRINSIC_% is deprecated by Figma anyway

what are your thoughts on this?

@marcomontalbano
Copy link
Owner

I would proceed with the option 2. Since INTRINSIC_% is deprecated, this way we'll not have to change the code in future Figma API releases.

@blinpete
Copy link
Contributor Author

blinpete commented Feb 7, 2024

@marcomontalbano
Yeah sure, just pushed a commit doing the 2nd option 👍

Copy link
Owner

@marcomontalbano marcomontalbano left a comment

Choose a reason for hiding this comment

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

Well done, thanks!

@marcomontalbano marcomontalbano merged commit 0fbc343 into marcomontalbano:main Feb 8, 2024
8 checks passed
@marcomontalbano marcomontalbano added the PR: New Feature 🚀 Only for pull request. New feature label Feb 8, 2024
@marcomontalbano
Copy link
Owner

I just released v4.8.0-alpha.1

Let me know if everything is working as expected.

@marcomontalbano marcomontalbano added this to the v4.8.0 milestone Feb 9, 2024
@blinpete
Copy link
Contributor Author

blinpete commented Feb 9, 2024

Yep, figma-export handles all the line heights as expected.
Thanks for such a quick merge 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: New Feature 🚀 Only for pull request. New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants