Skip to content

Conversation

@axkibe
Copy link
Contributor

@axkibe axkibe commented Feb 23, 2023

Description

adresses #574

How Has This Been Tested?

I tested it with my quite larger app, it works, it's quite a trivial change tough.

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.
    No I didn't, I tested it with my application.
  • My change requires a change to the documentation.
  • I have updated the README accordingly.
    charToGlyphIndex() and nameToGlyphIndex() are also not mentenioned in the README, maybe we should add all?
  • I have read the CONTRIBUTING document.

Copy link
Contributor

@yne yne left a comment

Choose a reason for hiding this comment

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

@axkibe could you add a test case in test/font.js ?

@Connum
Copy link
Contributor

Connum commented Feb 23, 2023

I also reviewed it, other than the missing test as mentioned, it looks good to me!

@axkibe
Copy link
Contributor Author

axkibe commented Feb 23, 2023

Ok I'll add the test tomorrow or the day after.

@axkibe
Copy link
Contributor Author

axkibe commented Feb 24, 2023

Ok, added.

@yne yne merged commit 427e88c into opentypejs:master Feb 24, 2023
@Connum Connum added this to the Release 2.0.0 milestone Feb 24, 2023
@yne yne mentioned this pull request Feb 24, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants