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

addition of typescript files #686

Closed
wants to merge 4 commits into from
Closed

Conversation

Layla-P
Copy link

@Layla-P Layla-P commented Jul 20, 2017

Description

A definition type file has been created for patternfly.js

Changes

All new files

attn:
@bleathem

@bleathem
Copy link
Member

This is great, thanks for the contribution @Layla-P! The typings are a good fit for this repo, as they will then be available for all consuming repos.

However the typescript api code introduced is not a good fit, as it doesn't reflect the programming model currently used in this repo.

Could you please remove the APIs and include only the typescript typings in this PR?

@Layla-P
Copy link
Author

Layla-P commented Jul 20, 2017

Hey @bleathem I have removed the files as requested. Are you happy with the typings files I have included? Or would you just like the patternfly one?

@bleathem
Copy link
Member

Thanks @Layla-P. That's an interesting question, let's see what our community has to say.

@dgutride @dlabrecq @priley86 thoughts on the ts typing contribution from @Layla-P?

@dlabrecq
Copy link
Member

dlabrecq commented Jul 20, 2017

I looked through the typescript definitions and there is a lot here. The definitions in patternfly.d.ts look like something we could pull into Patternfly because it's based on our scripts. I'm not sure about the definitions for jQuery, C3, D3, Datatables, and Bootstrap?

@Layla-P
Copy link
Author

Layla-P commented Jul 20, 2017

@dlabrecq I have written several typescript api's which make use of the type definitions such as a charting api. However, Brian asked me not to include them as he thought them not to be generic enough.
If the definitions aren't of use to you, that's no issue what so ever, I just thought I would share them and save someone the bother I've writing them as I have.
I have also written documentation on using the api's.

@dlabrecq
Copy link
Member

Can you please share the documentation? That would be very helpful. Thanks!

@Layla-P
Copy link
Author

Layla-P commented Jul 21, 2017

@dlabrecq I have emailed you on your redhat email.

@bleathem
Copy link
Member

Thanks again for the contribution @Layla-P. I discussed this PR with @dlabrecq and @jeff-phillips-18. While we feel the typings overall would be very useful for someone building there application with patternfly, the non-pattternfly typings would be better suited for a library like https://github.com/DefinitelyTyped/DefinitelyTyped.

The patternfly typing is however something that makes sense for use to host. Would you mind updating this PR to include only the patternfly typings file?

@bleathem
Copy link
Member

bleathem commented Aug 2, 2017

I added a commit to this PR removing the non-jquery typings. I will next send an e-mail to patternfly-list to kickstart a discussion around whether or not the community would find the inclusion of this typings file to be a valuable addition to patternfly.

@Layla-P
Copy link
Author

Layla-P commented Aug 3, 2017 via email

@dgutride
Copy link
Member

Replaced by #1043 - closing this PR - thanks for kicking off this conversation.

@dgutride dgutride closed this May 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants