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
compatible with react 18 types #8
compatible with react 18 types #8
Conversation
menglaiq-sportsbet
commented
Dec 18, 2023
- Update React to 18.2.0
- Update React types to 18.2.6
- Update to node 18.17.1 using nodenv
- Add customised children type to cater for React 18 types update ([react] React 18 types DefinitelyTyped/DefinitelyTyped#56210)
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.
LGTM
@@ -1,13 +1,13 @@ | |||
{ | |||
"name": "react-responsive-components", | |||
"version": "0.4.5", | |||
"version": "0.5.0", |
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.
No breaking changes on a major version upgrade of React and TS?
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.
Functionality wise we haven't noticed anything so far. It is only the typing.
ts/react-responsive-components.tsx
Outdated
// to pass a function as the children of the Responsive component. | ||
// Usually responsiveKey is a string type, but in your own repo, you may define a set of string as enum for its type. | ||
// Hence we use any here to cover any customised types. | ||
children?: React.ReactNode | undefined | ((responsiveKey: any) => JSX.Element) |
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.
Did we use any
all throughout for responsiveKey
?
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.
We definitely identified responsiveKey
as string
all throughout this library. If we use string
here does it not widen automatically when we pass ResponsiveKey
(type union of strings) to it?
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.
It should be string normally, what we did is we defined a type for it in our repo, like this:
export type ResponsiveKey = "narrowest" | "extraNarrow" | "narrow" | "medium" | "wide" | "widest"
But string should still work, let me change it to string
instead of any
.
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.
Updated.