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

Use line height based spacing model #4236

Closed
wants to merge 1 commit into from

Conversation

Enter-tainer
Copy link
Contributor

@Enter-tainer Enter-tainer commented May 23, 2024

fix #1028 fix #4224

The ci is expected to fail since the line height is changed, all of ref images are needed to be updated

TODO:

  • bring back orphan and widows prevention
  • fix TODOs. specifically about the spacing of term/enum/list --> How to calculate row gutter for grids
  • Find a good default for line height in normal text and footnote
  • figure out what it TIGHT_LEADING in math

@peng1999
Copy link
Contributor

  • Find a good default for line height in normal text and footnote

My suggestion is to find a value that keeps the line spacing unchanged in most tests...

@Enter-tainer
Copy link
Contributor Author

Enter-tainer commented May 24, 2024

  • Find a good default for line height in normal text and footnote

My suggestion is to find a value that keeps the line spacing unchanged in most tests...

image

It's impossible. Previously, the line height in typst is...(guess what?) 7.15(leading) + 7.240234375(asc) = 14.390234375pt. And this means that if we want it looks the same, the line-height would be 1.3082031250000001em or so.

But when i set it to 1.3082031250000001, the calculated leading is 7.150000000000002. And if i set it to the next float using np.nextafter, it becomes 7.149999999999999.

Anyway, i think it may not affect test a lot. I will set it to 1.308203125 at this moment

@Enter-tainer Enter-tainer force-pushed the mgt/line-height branch 4 times, most recently from 9052a0f to 9314792 Compare May 25, 2024 07:44
Comment on lines -169 to -185
let leading = if EquationElem::size_in(styles) >= MathSize::Text {
ParElem::leading_in(styles)
} else {
let font_size = scaled_font_size(ctx, styles);
TIGHT_LEADING.at(font_size)
};
Copy link
Contributor Author

@Enter-tainer Enter-tainer May 25, 2024

Choose a reason for hiding this comment

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

anyidea what is tight leading? Is it a workaround or something?

@LuxxxLucy
Copy link
Contributor

LuxxxLucy commented Jun 2, 2024

I just want to comment that #4318 may be a possible predecessor for the line height adjust solution can avoid breaking the test. This PR (#4318) is an attempt to support #2200 (I intend to do this and then refactor draft PR #3953), and I think it should be relevant so I think you might be interested to take a look at it.

@laurmaedje
Copy link
Member

laurmaedje commented Jul 20, 2024

I think until we have a solution that properly fixes the equation problem (for the first line and the last line as well), I'd refrain from making any changes. (In the interest of only changing things once.) So I'll close this for now.

@laurmaedje laurmaedje closed this Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-design This PR or issue is blocked by design work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: change leading option to line-height Large inline equations can overlap with other lines
4 participants