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

Converts settings to TypeScript #6315

Merged
merged 7 commits into from
Jan 17, 2020
Merged

Converts settings to TypeScript #6315

merged 7 commits into from
Jan 17, 2020

Conversation

Zyie
Copy link
Member

@Zyie Zyie commented Jan 7, 2020

Description of change

Opening another draft PR to let people know what is being worked on.

So far I have tried to use namespaces for settings however this resulted in many setting objects instead of one large one, not sure why.

For now I have gone with one interface that has optional parameters for the options defined outside the initial settings file.

Let me know if you guys have a better solution to try out. I dont think the current method is bad just not great.

Pre-Merge Checklist
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

@codecov-io
Copy link

codecov-io commented Jan 8, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #6315   +/-   ##
=======================================
  Coverage   77.24%   77.24%           
=======================================
  Files         178      178           
  Lines        9511     9511           
=======================================
  Hits         7347     7347           
  Misses       2164     2164
Impacted Files Coverage Δ
packages/prepare/src/BasePrepare.js 73.43% <ø> (ø) ⬆️

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 955f964...c133585. Read the comment docs.

@bigtimebuddy bigtimebuddy added this to the v5.3.0 milestone Jan 10, 2020
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.

I think this is fine. I'm not in love with the name PixiSettings, maybe simply Settings? We have bigger fish than settings. Lets get this in and we can try other approaches later.

@Zyie Zyie changed the title Start of converting settings to TypeScript Converts settings to TypeScript Jan 17, 2020
@Zyie Zyie marked this pull request as ready for review January 17, 2020 10:19
@Zyie
Copy link
Member Author

Zyie commented Jan 17, 2020

I'm not in love with the name PixiSettings, maybe simply Settings

I've updated the name

@ivanpopelyshev
Copy link
Collaborator

ivanpopelyshev commented Jan 17, 2020

@bigtimebuddy @Zyie What about IPixiSettings and IRenderOptions ? Also , RenderOptions in core will have more settings than that, and they will be optional. What about IBaseRenderOptions ?'

For example, it doesnt have resolution.

Copy link
Collaborator

@ivanpopelyshev ivanpopelyshev left a comment

Choose a reason for hiding this comment

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

I approve it anyway, because I can change RenderOptions in @pixi/core PR.

@Zyie
Copy link
Member Author

Zyie commented Jan 17, 2020

What about IPixiSettings and IRenderOptions ?'

Would it be a good idea to have a naming convention for interfaces. So all begin with I? That way we dont have a mashup of different names. For example we have IPoint, IDestoryOptions, but DecomposedDataUri, Settings, RenderOptions

For example, it doesnt have resolution.

Ah yes i didnt realise that got added dynamically in AbstractRenderer.

@ivanpopelyshev
Copy link
Collaborator

I think DecomposedDataUri is the only one without I

@bigtimebuddy
Copy link
Member

I like using I as a prefix for interfaces.

@Zyie
Copy link
Member Author

Zyie commented Jan 17, 2020

Ok I've updated the names to use the prefix. I'll let @ivanpopelyshev fix whatever needs to be fixed in his core pr

@bigtimebuddy bigtimebuddy merged commit 60669e0 into dev Jan 17, 2020
@bigtimebuddy bigtimebuddy deleted the dev-settings-typescript branch January 17, 2020 14:05
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