-
-
Notifications
You must be signed in to change notification settings - Fork 32k
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
[docs] Migrate DefaultTheme demo to Typescript #16920
Conversation
@@ -78,7 +81,1685 @@ function DefaultTheme(props) { | |||
|
|||
DefaultTheme.propTypes = { | |||
classes: PropTypes.object.isRequired, | |||
theme: PropTypes.object.isRequired, | |||
theme: PropTypes.shape({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how to "fix" this, it's transpiled like this because I'm using the Theme
type in Props
, and that's correct, but now the javascript code is pretty much unreadable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to tell it to not resolve props with the name theme
, same as what was done with classes
https://github.com/mui-org/material-ui/blob/0168a6d2ecb7c684dd12a7ea8e5cf210f6ee4944/docs/scripts/formattedTSDemos.js#L91-L93
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! That will be very useful in the future.
No bundle size changes comparing 78db8a0...82cd50b |
6c789c3
to
82cd50b
Compare
@rogerclotet Hey, thank you for taking the time to work on this issue! However, I don't think that it was needed. The sources of the demo are not visible ( @eps1lon Do you think that we should accept the changes? I'm happy either way. |
@oliviertassinari I looked for a somehow easy to migrate component from the list in #14897 for my first contribution, since I'm not experienced and I want to start contributing a bit more. I don't mind if this is closed, just make sure the issue is properly updated to indicate this component migration is not necessary. Regarding |
@rogerclotet You are right. Thank you for pointing that out. I have updated the related issue to remove the demos that have the "see source" button hidden. I have confused What do you think of changing the flag of the demo? As for the TypeScript version of it, if it was simple to migrate. I think that we should keep the JavaScript one. The source should be optimize for the speed of iteration of the contributors. It's probably faster to work with the JavaScript version. |
Thanks for your interest in contributing 🤗. We only have a few demos left to migrate. If you could manage to get them all, it would be awesome 💪. |
Seems fair, it wasn't too hard to migrate, and the transpiled code is a mess. I will close this PR and open a new one adding the I will also start migrating other demos, I will try to find some more useful ones next time 😉 Thank you for your feedback! |
Opened a new PR: #16937 |
Add Typescript Demo (#14897) for DefaultTheme.