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

fix: correctly generate numeric unicode subset faces [#949] #961

Merged
merged 13 commits into from Apr 16, 2024

Conversation

RBilly
Copy link
Contributor

@RBilly RBilly commented Apr 15, 2024

Template from packages/cli/src/sass/mixins.ts file make use of the metada file generated by packages/cli/src/sass/metada.ts. In this metada file, each subset of a font is represented by a unicode subset defined by a unicode range but sometime, the main font subset can be represented by numeric subsets.
When calling the mixin function with this main subset specified as parameter, there is an issue where the mixin function tries to verify that this main subset is equal to the default subset. But this is rarely the case because most of the time the default subset is almost always set to latin, so it fails to produce the expected output.

Change made in packages/cli/src/sass/mixins.ts check that if the the subset value passed as parameter does not have a unicode subset but is part of the list of the font subsets then we use the numeric unicode subsets.

Test case was update to be able to pass options to the mixin call and catch this case as well.

side note: indentation of some files were also modified but it does not seem like an error since they seem to be aligned with the content of the editorconfig file.

Copy link

changeset-bot bot commented Apr 15, 2024

🦋 Changeset detected

Latest commit: eb239d6

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

This PR includes changesets to release 2 packages
Name Type
@fontsource-utils/cli Patch
cdn 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

Copy link
Member

@ayuhito ayuhito left a comment

Choose a reason for hiding this comment

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

I think everything looks good. This shouldn't be breaking, right?

packages/cli/src/sass/mixins.ts Outdated Show resolved Hide resolved
@ayuhito ayuhito linked an issue Apr 15, 2024 that may be closed by this pull request
@RBilly
Copy link
Contributor Author

RBilly commented Apr 16, 2024

I think everything looks good. This shouldn't be breaking, right?

I do not consider this a breaking change but there is some impact.
Previously, when calling the faces mixin without a subsets parameter, it would use the default subset instead. So it would generate the numeric unicode faces + latin.
With my change I could either let the default subset be used but then it would only generate "latin" faces when no subsets were provided which does not make sense for a lot of fonts that have latin as default. Or I could just say that if no subsets are provided, it means all subsets should be used.
I chose the later but it means that for some user they could import more faces than before which is more correct but it could unexpectedly increase their bundle size.

@ayuhito
Copy link
Member

ayuhito commented Apr 16, 2024

I chose the later but it means that for some user they could import more faces than before which is more correct but it could unexpectedly increase their bundle size.

Widening the subset range is fine since users won't necessarily download the extra font files unless their webpage uses unicode characters outside the latin subset.

Thank you very much for your contribution! I think it looks good to me! 🎉

@ayuhito ayuhito merged commit ae11885 into fontsource:main Apr 16, 2024
1 check passed
@github-actions github-actions bot mentioned this pull request Apr 16, 2024
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.

Chinese Noto fonts mixin does not allow using symbolic subsets
2 participants