Convert Text to TypeScript #6390
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #6390 +/- ##
=======================================
Coverage 74.83% 74.83%
=======================================
Files 66 66
Lines 3346 3346
=======================================
Hits 2504 2504
Misses 842 842Continue to review full report at Codecov.
|
packages/text/src/Text.ts
Outdated
| /** | ||
| * @param {string} text - The string that you would like the text to display | ||
| * @param {object|PIXI.TextStyle} [style] - The style parameters | ||
| * @param {HTMLCanvasElement} [canvas] - The canvas element for drawing text | ||
| */ | ||
| constructor(text, style, canvas) | ||
| constructor(text: string, style: TextStyle, canvas: HTMLCanvasElement) |
There was a problem hiding this comment.
Style type should be ITextStyle instead TextStyle, because we can pass RAW object to constructor together with TextStyle instance.
There was a problem hiding this comment.
Yes it should say Partial<ITextStyle>|TextStyle like elsewhere. I missed that.
| @@ -4,7 +4,39 @@ | |||
| import { TEXT_GRADIENT } from './const'; | |||
| import { hex2string } from '@pixi/utils'; | |||
|
|
|||
| const defaultStyle = { | |||
| export interface ITextStyle { | |||
There was a problem hiding this comment.
ALL fields is optional and has default values.
There was a problem hiding this comment.
Yes. I made the fields mandatory to force all of the values to be provided in the default values. I've been using Partial<ITextStyle> in places where they're optional. If using Partial is not something you would like to have there I can definitely change that.
There was a problem hiding this comment.
Also having the fields optional wouldn't enforce TextStyle implements ITextStyle properly now that I added it.
packages/text/src/TextStyle.ts
Outdated
| @@ -773,7 +847,7 @@ function getColor(color) | |||
| * @param {Array} array2 Second array to compare | |||
| * @return {boolean} Do the arrays contain the same values in the same order | |||
| */ | |||
| function areArraysEqual(array1, array2) | |||
| function areArraysEqual(array1: any[], array2: any[]): boolean | |||
There was a problem hiding this comment.
I think it should be generic function
There was a problem hiding this comment.
Changing this to function areArraysEqual<T1, T2>(array1: T1[], array2: T2[]): boolean results in a problem with the !== comparison: This condition will always return 'true' since the types 'T1' and 'T2' have no overlap.
I can change it to function areArraysEqual<T>(array1: T[], array2: T[]): boolean but then it cannot be called with different types of arrays. Then again maybe you don't need to because if they're different types then they definitely are not equal. I'll go with this one.
There was a problem hiding this comment.
Why need to compare arrays with different type?
It used only in fillGradientStops setter, gradient stops is a number type, isn't it?
https://github.com/quicksave-interactive/pixi.js/blob/dev/packages/text/src/TextStyle.ts#L415
It should be:
function areArraysEqual(array1: number[], array2: number[]): boolean
packages/text/src/TextMetrics.ts
Outdated
| @@ -539,7 +572,7 @@ export class TextMetrics | |||
| * @param {boolean} breakWords The style attr break words | |||
| * @return {boolean} whether to break word or not | |||
| */ | |||
| static canBreakWords(token, breakWords) | |||
| static canBreakWords(token: string, breakWords: TextStyle['breakWords']): boolean | |||
There was a problem hiding this comment.
I think types should be explicit instead of lookups.
packages/text/src/TextStyle.ts
Outdated
| @@ -57,6 +89,38 @@ const genericFontFamilies = [ | |||
| */ | |||
| export class TextStyle | |||
| { | |||
| public styleID: number; | |||
|
|
|||
| protected _align: ITextStyle['align']; | |||
There was a problem hiding this comment.
I think is a bad practice using lookup types in open source project.
There was a problem hiding this comment.
I can get rid of those. I agree that it's not really nice for classes to reference each other, but given that TextStyle implements ITextStyle I figured it would make sense to specify the types in only one place. But I'll change those to not use lookup, I'll just need to declare a bunch of extra types for the interface so I don't have to type for example 'normal'|'bold'|'bolder'|'lighter'|'100'|'200'|'300'|'400'|'500'|'600'|'700'|'800'|'900' more than once.
There was a problem hiding this comment.
Create separate types for things like this maybe? TextStyleAlign, TextStyleWeight, etc
There was a problem hiding this comment.
Yep that's what I did. I didn't have the TextStyle prefix but I just added that to all of them - I think that makes it a bit clearer. I'll push in a bit.
packages/text/src/Text.ts
Outdated
| @@ -383,7 +401,7 @@ export class Text extends Sprite | |||
| * @param {PIXI.Rectangle} rect - The output rectangle. | |||
| * @return {PIXI.Rectangle} The bounds. | |||
| */ | |||
| getLocalBounds(rect) | |||
| getLocalBounds(rect: Rectangle): Rectangle | |||
There was a problem hiding this comment.
'public' modifier is required!
empty modifier only for 'internal' (not public API, but used outside of type, such as inside Renderer )
There was a problem hiding this comment.
Ah okay. I missed that there is a semantic difference for this, I'll check the visibility for all the methods.
|
Cool start!
|
|
@Zyie you good on this? |
seems good to me |
Description of change
Convert Text to TypeScript
Pre-Merge Checklist
npm run lint)npm run test)