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 setting preferred subfamily #635

Merged
merged 4 commits into from
Nov 11, 2023

Conversation

StreakInTheSky
Copy link
Contributor

@StreakInTheSky StreakInTheSky commented Nov 4, 2023

Description

Fixed how a font's preferredSubfamily is set.
Currently in the tables/sfnt.js file inside fontToSfntTable, when setting a font's preferredSubfamily it's looking for preferredSubFamily, so if it's not already set it will always be undefined. Changed the F to an f

Motivation and Context

Fixes undefined preferredSubfamily if it's not explicitly set

How Has This Been Tested?

Testing a feature from this repo/site. which parses font files which does not have macintosh.preferredSubfamily set, but has windows.preferredSubfamily set. Specifically fonts from here. Launched the site locally using a simple http server and ran the customization feature from the site, while testing how the font object's name props change.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I did npm run test and all tests passed green (including code styling checks).
  • I have added tests to cover my changes.
  • My change requires a change to the documentation.
  • I have updated the README accordingly.
  • I have read the CONTRIBUTING document.

@Connum
Copy link
Contributor

Connum commented Nov 4, 2023

Thanks for your contribution! I think the first link is incorrect, because it links to the repo root of the font of your second link, and not some testing tool.

Could you also add a test case for this issue, please?

@StreakInTheSky
Copy link
Contributor Author

Sorry, didn't really write a standalone script or test to test this out. Since I was trying to fix a particular issue in that repo, just did it manually through my fork of that site.

But I can write a test case for it. I do want to clarify of how I should go about it. I was thinking of creating a new test file for sfnt.js, And then just make some test instances of Font with the relevant data inside the names property. Then I'd test if the Font's name values are correct. after running through the function.

@Connum
Copy link
Contributor

Connum commented Nov 4, 2023

Basically we want to test fontToSfntTable in sfnt.js, but there's a wrapper on the Font object called toTables, so I think we could add it there as well. There is already a test making sure that table data is being serialized, the code of which is a good example of how to access the resulting structure. The new test should then get the name table and check for the correct value.

@StreakInTheSky
Copy link
Contributor Author

Hi, so I added another script in package.json to just run mocha so I could specify the test files to run. Would you like this committed or named something else?

"scripts": {
    ...,
    "test": "npm run build && npm run dist && mocha --require reify --recursive && npm run lint",
    "test:local": "mocha --require reify --recursive",
    ...
},

But as an update, I finally got the tables parsed correctly so I can check the values for equality. Hopefully can get this done in the next day or so.

@Connum
Copy link
Contributor

Connum commented Nov 8, 2023

Would you like this committed or named something else?

Let's not include that with this PR

@StreakInTheSky
Copy link
Contributor Author

Hey sorry, took a bit longer. Pushed the tests for review.

Copy link
Contributor

@Connum Connum left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@Connum Connum merged commit 88878ea into opentypejs:master Nov 11, 2023
1 check passed
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.

2 participants