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

Support font-weight, font-style and font-stretch @font-face properties #264

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

Conversation

kilobyte2007
Copy link

Support font-weight, font-style, and font-stretch @font-face properties as discussed in #53

# Conflicts:
#	playground/index.html
#	src/transform.ts
#	test/e2e.spec.ts
@kilobyte2007
Copy link
Author

I see there's no enough test coverage for the new code I suppose but I am not entirely sure what tests should I implement. Can you point me, please?

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

This looks really good. But it doesn't seem to be working in the playground (as in, actually reducing layout shift). Am I just missing something?

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (9099053) 100.00% compared to head (372edc3) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #264   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          197       230   +33     
  Branches        31        37    +6     
=========================================
+ Hits           197       230   +33     
Files Changed Coverage Δ
src/css.ts 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kilobyte2007
Copy link
Author

@danielroe looking into this now, thanks for the tweaks!

@kilobyte2007
Copy link
Author

I have missed the Roboto and Inter font-face definitions in the index.css file, which seems to have fixed the issue in part, but now the issue is that capsize sets the same metrics for the bold font as it sets for the non-bold one (which I think is correct if I verify the numbers here - https://seek-oss.github.io/capsize/).

Does this mean that this PR is useless and the way it worked previously is enough?

@font-face {
  font-family: "Inter fallback";
  src: local("Arial");
  size-adjust: 107.4014%;
  ascent-override: 90.199%;
  descent-override: 22.4836%;
  line-gap-override: 0%;
}
@font-face {
  font-family: 'Inter';
  font-display: swap;
  src: url('https://fonts.gstatic.com/s/inter/v12/UcC73FwrK3iLTeHuS_fvQtMwCp50KnMa1ZL7.woff2')
    format('woff2');
}

@font-face {
  font-family: "Inter fallback";
  src: local("Arial");
  size-adjust: 107.4014%;
  ascent-override: 90.199%;
  descent-override: 22.4836%;
  line-gap-override: 0%;
  font-weight: 700;
}
@font-face {
  font-family: 'Inter';
  font-weight: 700;
  font-display: swap;
  src: url('https://fonts.gstatic.com/s/inter/v12/UcCO3FwrK3iLTeHuS_fvQtMwCp50KnMw2boKoduKmMEVuFuYAZ9hiJ-Ek-_EeA.woff2')
    format('woff2');
}

Copy link
Member

danielroe commented Aug 17, 2023

This may be an upstream issue to raise in capsize in that case. We can pend this PR until there are different metrics available for different weights.

@DamianGlowala
Copy link

In theory, there is a way to obtain different metrics for different font weights e.g. through dropping each font file here and collecting relevant metrics from the 'Data' tab. However, as Daniel mentioned, it would be much nicer if capsize provided these alongside the already existing metrics for the normal font weight.

Copy link

codecov bot commented Feb 23, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.

Project coverage is 54.08%. Comparing base (04c5b8e) to head (a35efe3).

Files with missing lines Patch % Lines
src/transform.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #264      +/-   ##
==========================================
+ Coverage   50.21%   54.08%   +3.87%     
==========================================
  Files           4        4              
  Lines         235      257      +22     
  Branches       33       40       +7     
==========================================
+ Hits          118      139      +21     
- Misses        117      118       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

Successfully merging this pull request may close these issues.

4 participants