Skip to content

[docs] Add Typescript demos for MenuPopupState #17869

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

Closed
wants to merge 2 commits into from
Closed

[docs] Add Typescript demos for MenuPopupState #17869

wants to merge 2 commits into from

Conversation

programistka
Copy link
Contributor

@programistka programistka commented Oct 14, 2019

  • [x ] I have followed (at least) the PR section of the contributing guide.
    I wasn't able to run successfully yarn docs:typescript:check as it was told that it can be pushed anyway. Please let me know if I did it right or wrong.

Related to #14897

@eps1lon eps1lon mentioned this pull request Oct 14, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Oct 14, 2019

No bundle size changes comparing 6b9ecbe...3907aea

Generated by 🚫 dangerJS against 3907aea

@eps1lon eps1lon added docs Improvements or additions to the documentation typescript labels Oct 14, 2019
@eps1lon
Copy link
Member

eps1lon commented Oct 14, 2019

Thanks for working on this.

material-ui-popup-state does not have types (yet). You can start by adding the required types to docs/types/material-ui-popup-state.d.ts (the folder has some examples). Once the tests pass we can think about contributing these types to the appropriate @types/* package hosted in https://github.com/DefinitelyTyped/DefinitelyTyped

@programistka
Copy link
Contributor Author

@eps1lon Thanks for the detailed instruction. However I tried to follow it and there is stil an issue. What I'm doing wrong?

@eps1lon
Copy link
Member

eps1lon commented Oct 14, 2019

What I'm doing wrong?

You need to add actual types

declare module 'material-ui-popup-state' {
  // export vars, functions, types go here
}

https://medium.com/@chris_72272/migrating-to-typescript-write-a-declaration-file-for-a-third-party-npm-module-b1f75808ed2 looks like a good guide (skip to "Create the declaration file")

@oliviertassinari
Copy link
Member

Assuming the long term plan is to remove the need for this package (with simpler bindings in the core), would it make sense to ignore the migration of this demo to TypeScript? It's an open question, I think that it depends on how easy the migration of the demo is, how long we will keep it, and how many people use it.

@oliviertassinari
Copy link
Member

We had the following mention in #14897 a week ago:

[x] MenuPopupState (⚠️ to use the new hook API of the third party library) no types for material-ui-popup-state

@oliviertassinari oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Oct 18, 2019
@oliviertassinari
Copy link
Member

How guys do you want to move forward with this problem?

@oliviertassinari
Copy link
Member

Ok, let's optimize for the long term. I think that the optimal outcome would be to solve the problem https://github.com/jcoreio/material-ui-popup-state solves in the core directly. I'm closing the pull request, @programistka thank you so much for investing time in it! Hopefully, it was a worthy experience.

In the meantime, if jcoreio/material-ui-popup-state#12 get resolved, I think that can accept a pull request with a TypeScript version of the demos.

@jedwards1211
Copy link
Contributor

jedwards1211 commented Dec 12, 2019

Hi folks, I've finally gotten enough experience with TS to be comfortable adding TS types to material-ui-popup-state, it just landed in 1.5.0 😄 I did check my defs with tsc, but hopefully there are no issues with actually consuming the types.

@oliviertassinari
Copy link
Member

Nice :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants