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 Graphics to typescript #6352

Merged
merged 27 commits into from
Feb 3, 2020
Merged

Convert Graphics to typescript #6352

merged 27 commits into from
Feb 3, 2020

Conversation

eXponenta
Copy link
Contributor

@eXponenta eXponenta commented Jan 26, 2020

Description of change

Convert PIXI.Graphics to TS

P.S Awaits before core's problems will be resolved.

cross-package changes:

type IShape = Circle | Ellipse | Polygon | Rectangle | RoundedRectangle;- moved to @pixi/math

Transform._worldID - is internal (without modificator)

changed signature of hex2rgb to:

hex2rgb(hex: number, out: Array<number> | Float32Array = []): Array<number> | Float32Array

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)

@ivanpopelyshev
Copy link
Collaborator

It has begun.

@codecov-io
Copy link

codecov-io commented Jan 27, 2020

Codecov Report

Merging #6352 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #6352   +/-   ##
=======================================
  Coverage   75.03%   75.03%           
=======================================
  Files          86       86           
  Lines        4819     4819           
=======================================
  Hits         3616     3616           
  Misses       1203     1203

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 965ee03...9fc888e. Read the comment docs.

@bigtimebuddy bigtimebuddy added this to the v5.3.0 milestone Jan 27, 2020
@eXponenta eXponenta marked this pull request as ready for review January 31, 2020 20:04
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.

Looks good. Just a couple of things to discuss

packages/graphics/src/Graphics.ts Outdated Show resolved Hide resolved
packages/graphics/src/Graphics.ts Outdated Show resolved Hide resolved
packages/graphics/src/Graphics.ts Outdated Show resolved Hide resolved
packages/graphics/src/Graphics.ts Outdated Show resolved Hide resolved
packages/graphics/src/GraphicsData.ts Outdated Show resolved Hide resolved
packages/graphics/src/GraphicsData.ts Show resolved Hide resolved
packages/graphics/src/GraphicsGeometry.ts Show resolved Hide resolved
@bigtimebuddy
Copy link
Member

All the chain-able methods should return this Not Graphics, eg lineStyle method.

packages/graphics/src/Graphics.ts Outdated Show resolved Hide resolved
packages/graphics/src/Graphics.ts Show resolved Hide resolved
@eXponenta
Copy link
Contributor Author

eXponenta commented Feb 1, 2020

All the chain-able methods should return this Not Graphics, eg lineStyle method.

What you mean?

this is Graphics type.
All chain methods return this.
Chain test is passed.

Linter breaks without assignments type of return.

@Zyie
Copy link
Member

Zyie commented Feb 1, 2020

All the chain-able methods should return this Not Graphics, eg lineStyle method.

What you mean?

He means that a method that returns the instance of the class should use this instead of Graphics.

e.g. public lineStyle(options: ILineStyleOptions | number = null): this

@eXponenta
Copy link
Contributor Author

All the chain-able methods should return this Not Graphics, eg lineStyle method.

What you mean?

He means that a method that returns the instance of the class should use this instead of Graphics.

e.g. public lineStyle(options: ILineStyleOptions | number = null): this

Thanks, done.

Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

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

Why did you remove buildComplexPoly and bezierCurveTo functions?

packages/graphics/src/Graphics.ts Outdated Show resolved Hide resolved
packages/graphics/src/Graphics.ts Outdated Show resolved Hide resolved
packages/graphics/src/Graphics.ts Outdated Show resolved Hide resolved
packages/graphics/src/Graphics.ts Outdated Show resolved Hide resolved
}

if (!Array.isArray(points))
else
{
Copy link
Member

Choose a reason for hiding this comment

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

Why remove support for ...args:number[] here? These TS conversions shouldn't remove functionality. I suggest you add another argument signature for args implementation.

Copy link
Contributor Author

@eXponenta eXponenta Feb 1, 2020

Choose a reason for hiding this comment

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

Same problem in Poligon package now. We doesn't support arrays in constructor now.
Can you write 2 annotation with ... and [], because i wasn't resolve this?

And we can't do this because there is lint rule as no-dupe-class-members

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oke, i restore it.
Tests will come later.

packages/graphics/src/styles/FillStyle.ts Show resolved Hide resolved
@eXponenta
Copy link
Contributor Author

Why did you remove buildComplexPoly and bezierCurveTo functions?

Dead and doublicated code.
There aren't usage of it, and wasn't public because there no any exports.
Then buildComplexPoly is broken becouse used deaded render features, bezierCurveTo is same as BezierUtils.curveTo.

eXponenta and others added 3 commits February 1, 2020 21:53
Co-Authored-By: Matt Karl <matt@mattkarl.com>
Co-Authored-By: Matt Karl <matt@mattkarl.com>
@eXponenta
Copy link
Contributor Author

eXponenta commented Feb 1, 2020

Conclusion:

  • public readonly geometry can't be used because destroy method assign null to them, of course we always can do (this as any).geometry = null but what for?
  • we can't create more that one method/constructor according to lint rule and some collaborators don't like this crutch ( such as me or @ivanpopelyshev )

upd. @andrewstart change lint rule, now we can do multiple annotations

upd:
public readonly geometry -> public get geometry() & private _geometry, because otherwice we can't assign null in destroy method

Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@bigtimebuddy bigtimebuddy merged commit 74f131e into dev Feb 3, 2020
@bigtimebuddy bigtimebuddy deleted the graphics-ts branch February 3, 2020 11:58
@bigtimebuddy
Copy link
Member

Thank you @eXponenta 🎉

@ivanpopelyshev
Copy link
Collaborator

@bigtimebuddy do you even sleep?

@bigtimebuddy
Copy link
Member

@ivanpopelyshev newborn duty 👶

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

6 participants