-
-
Notifications
You must be signed in to change notification settings - Fork 706
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Font features #642
Font features #642
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hefler Can we update one of the integration tests (for all 4 environments) to pass a font feature to disable ligatures (per #490)? That way we know this works properly and it won't be broken in the future.
Also, notice that the pipeline for this PR failed. The pipeline must pass before this can be merged. Looks like it was the yarn typecheck
command that failed.
package.json
Outdated
"@rollup/plugin-commonjs": "^13.0.0", | ||
"@rollup/plugin-json": "^4.1.0", | ||
"@rollup/plugin-node-resolve": "^8.0.1", | ||
"@types/jest": "^26.0.0", | ||
"@types/node-fetch": "^2.5.7", | ||
"@types/pako": "^1.0.1", | ||
"@zerollup/ts-transform-paths": "^1.7.18", | ||
"downlevel-dts": "^0.5.0", | ||
"downlevel-dts": "^0.6.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be updated as part of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest I don't know, I believe this was caused by an audit fix
.
I'm also not super familiar writing tests either, but I can give it a go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case I'd like to undo this change. If the dependency needs to be updated, I'd prefer that to be a separate PR. But I don't think it's necessary for the changes you're making anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, with respect to testing, no worries if you're new to it! I'd recommend you read over the CONTRIBUTING.md
file. All contributors should read it. There are sections explaining how to run the unit tests, integration tests (though you don't need to worry about running the React Native tests), and running the typechecker (amongst other important things). Let me know if you have any questions.
* Font features (#642) * Font features option for embedded fonts (#490) * Updated downlevel-dts * Typing `fontFeatures` to TypeFeatures * Reverting to `downlevel-dts@^0.5.0` * Updated EmbedFont Options with TypeFeatures * Add a test for TypeFeatures option * Update yarn.lock * Cleanup font features implementation * Cleanup Co-authored-by: Felipe Hefler <hefler@users.noreply.github.com>
@hefler Thanks for working on this! These changes will go out in the next release of pdf-lib! |
@Hopding No worries! Also thanks for the patience and guidance ✌️ |
* Font features (#642) * Font features option for embedded fonts (#490) * Updated downlevel-dts * Typing `fontFeatures` to TypeFeatures * Reverting to `downlevel-dts@^0.5.0` * Updated EmbedFont Options with TypeFeatures * Add a test for TypeFeatures option * Update yarn.lock * Cleanup font features implementation * Cleanup Co-authored-by: Felipe Hefler <hefler@users.noreply.github.com>
I hope this helps mitigating issue #490.