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

Convert text-bitmap package to TypeScript #6479

Merged
merged 27 commits into from
Mar 28, 2020
Merged

Convert text-bitmap package to TypeScript #6479

merged 27 commits into from
Mar 28, 2020

Conversation

lloydevans
Copy link
Contributor

@lloydevans lloydevans commented Mar 17, 2020

Description of change

Convert text-bitmap package to TypeScript.

Pre-Merge Checklist
  • Tests and/or benchmarks are included
  • Documentation is changed or added
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

@bigtimebuddy
Copy link
Member

Alright @lloydevans! Just ping us when this is ready for review or if you have any questions. Lots of packages have been completed so far, check out text, which is pretty close to this one.

@lloydevans
Copy link
Contributor Author

@bigtimebuddy Thanks! will do. Yea did have a couple of questions but looking at the text PR and existing packages answered things.

@lloydevans
Copy link
Contributor Author

Hey @bigtimebuddy,

I think I've done a good chunk of the conversion here. Some of it needs slightly bigger refactoring work to get full type coverage. I've opted for casting things to any over refactoring code just to silence typescript errors for now.

BitmapFont.ts - Converted fine for the most part, with the need to cast data going to the auto-selected format parser as any.

BitmapFontData.ts - The JSDoc comments mark properties as readonly, but it is changed from outside of the class in the format parsers. Maybe this only needs to be an interface now?

BitmapFontLoader.ts - Some need for any around the resource object as the custom bitmap font loader adds some additional properties there.

BitmapText.ts - Fine for the most part, but there are two things. One is the _font property has a get/set system which can accept a string or object. I think I've seen discussions on this elsewhere, relating to get/set function signatures matching. Did you come to a solution on that? Same goes for the anchor, in JSDoc it is either a number or point, elsewhere I am seeing it is ObservablePoint. Should I make it ObservablePoint here?

BitmapTextStyle.ts - This is a new file I've added with some interfaces for text style. Would you prefer these interface/types live at the top of the BitmapText.ts file?

TextFormat.ts - This file has a few TS issues with the way it's accessing properties on the bitmap font data object from string attribute names. It is also using parseInt to both cast to number and floor I believe, but TypeScript errors passing a number to parseInt. I've cast things to any for now, but could possibly look at doing a bit of refactoring here to avoid that.

XMLFormat.ts - Apart from the wider point about the format classes, this one converted fine.

@codecov-io
Copy link

codecov-io commented Mar 22, 2020

Codecov Report

Merging #6479 into dev will decrease coverage by 0.52%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #6479      +/-   ##
==========================================
- Coverage   83.01%   82.48%   -0.53%     
==========================================
  Files          45       38       -7     
  Lines        2272     1907     -365     
==========================================
- Hits         1886     1573     -313     
+ Misses        386      334      -52     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cec804...03cd2df. Read the comment docs.

@lloydevans lloydevans marked this pull request as ready for review March 27, 2020 14:26
@bigtimebuddy bigtimebuddy requested a review from Zyie March 27, 2020 18:38
Copy link
Member

@Zyie Zyie left a comment

Choose a reason for hiding this comment

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

Two small comments but otherwise looks good!

packages/text-bitmap/src/BitmapText.ts Outdated Show resolved Hide resolved
packages/text-bitmap/src/BitmapFont.ts Outdated Show resolved Hide resolved
@bigtimebuddy bigtimebuddy merged commit cd8dffb into pixijs:dev Mar 28, 2020
@bigtimebuddy
Copy link
Member

Thank you @lloydevans!

@lloydevans lloydevans deleted the dev-typescript-text-bitmap branch March 28, 2020 10:54
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.

None yet

4 participants