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

chore(typings): add basic typings #53

Merged
merged 5 commits into from
Dec 5, 2018
Merged

Conversation

bigopon
Copy link
Contributor

@bigopon bigopon commented Dec 3, 2018

Checklist
  • npm run test passes
  • tests and/or benchmarks are included for the features/bug
  • documentation is changed or added

One thing that doesn't feel right to me is there is no way to tell if a node is UiControl, I think it would be needed from times to times when integrated into other higher level templating frameworks. At the moment, I kind of have to do this:

const UiControl = Object.getPrototypeOf(UiLabel);

Maybe consider start exporting UiControl? For the typings, UiControl, UiEntryBase, UiBox are marked as abstract class and should be enough to prevent TypeScript user to avoid calling it directly.

@bigopon
Copy link
Contributor Author

bigopon commented Dec 3, 2018

@bigopon
Copy link
Contributor Author

bigopon commented Dec 3, 2018

There are classes / APIs without any comments so I'm not sure how to add. i.e UiDialogs, UiDrawMatrix

@bigopon
Copy link
Contributor Author

bigopon commented Dec 3, 2018

The typings work is complete from my side. There could be incorrect typings around matrix / drawing classes.

@parro-it
Copy link
Owner

parro-it commented Dec 4, 2018

Thank you, @bigopon, good job!

One thing that doesn't feel right to me is there is no way to tell if a node is UiControl

I'll open a specific issue to remember directly exporting UiControl.

There are classes / APIs without any comments so I'm not sure how to add. i.e UiDialogs, UiDrawMatrix

I'll add comment for them, we can improve typing after that.

Can you add the new file to the "files" section of package.json, so it will be published to npm?

@bigopon
Copy link
Contributor Author

bigopon commented Dec 4, 2018

@parro-it Added 👍

I'll add comment for them, we can improve typing after that.

Awesome.

@mischnic
Copy link
Contributor

mischnic commented Dec 4, 2018

I'll make some correction after this gets merged, but in the big picture @parro-it:

I guess the type definitions shouldn't include deprecated & undocumented parts (like unscoped enums).
Also: the setters and getter function we are adding dynamically are missing.

@mischnic
Copy link
Contributor

mischnic commented Dec 4, 2018

@bigopon It the public modifier for properties needed? It's not used consistently.

@parro-it
Copy link
Owner

parro-it commented Dec 4, 2018

Also: the setters and getter function we are adding dynamically are missing.

I'm not sure if they can be dynamically added to the TS types too...

@bigopon: basically, for each control property, we dinamically create and expose two methods: a getter and a setter.

@bigopon
Copy link
Contributor Author

bigopon commented Dec 4, 2018

@mischnic Everything is public by default, probably i overlooked while copypasting from the other PR. For those getter setter, i can do another commit, didn't look carefully

deprecated & undocumented parts (like unscoped enums).

Can you help clarify which ones, I was unsure about the enums and mostly copypasted them

@mischnic
Copy link
Contributor

mischnic commented Dec 4, 2018

Everything is public by default, probably i overlooked while copypasting from the other PR.

Ok

Can you help clarify which ones, I was unsure about the enums and mostly copypasted them

I'm going fix that in a followup PR.

@bigopon
Copy link
Contributor Author

bigopon commented Dec 4, 2018

Cool. If @parro-it and @mischnic prefer, maybe call it a day with this PR and let @mischnic do the correction work? I can do another commit too, wouldnt make any difference for me

@parro-it
Copy link
Owner

parro-it commented Dec 5, 2018

Ok, so I merge this and we'll do the corrections in other PR.

@parro-it parro-it merged commit 8e959e6 into parro-it:master Dec 5, 2018
@bigopon
Copy link
Contributor Author

bigopon commented Dec 5, 2018

Nice, looking for ui table and typings in the new release 🍻

@mischnic mischnic mentioned this pull request Dec 5, 2018
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.

3 participants