-
Notifications
You must be signed in to change notification settings - Fork 2
Typescript conversion #1
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: master
Are you sure you want to change the base?
Conversation
} | ||
|
||
export interface ResponsivePropsConfig { | ||
propKeys: Array<string>; |
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.
Array<T>
is used a couple times in this PR, but it's generally better to use T[]
.
@@ -17,11 +23,11 @@ ResponsiveProvider.propTypes = { | |||
/** The names and values of the responsive breakpoints */ | |||
thresholdMap: PropTypes.object, | |||
/** @ignore */ | |||
children: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.node), PropTypes.node]).isRequired | |||
children: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.node), PropTypes.node]).isRequired, |
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.
Shouldn't propTypes
be replaced with interfaces?
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.
Technically they are not identical and still have value in non-TS apps. I was trying to change as little functionality as possible in this PR.
"esModuleInterop": true, | ||
"preserveSymlinks": true, | ||
"baseUrl": ".", | ||
"outDir": "./dist" |
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.
Does this build to both common-js and es-modules?
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.
only cjs
Co-authored-by: Jordan Janzen <jordancjanzen@gmail.com>
Co-authored-by: Jordan Janzen <jordancjanzen@gmail.com>
Co-authored-by: Jordan Janzen <jordancjanzen@gmail.com>
Co-authored-by: Jordan Janzen <jordancjanzen@gmail.com>
Added config to build and test Typescript.
Converted src to TS (but not the tests yet).