Update type react-toastr: Allow JSX to be used for title and message content.#24910
Conversation
|
@DanRegazzi Thank you for submitting this PR! 🔔 @shssoichiro - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
types/react-toastr/index.d.ts
Outdated
| info: (message: string, title: string, optionsOverride?: {}) => void; | ||
| success: (message: string, title: string, optionsOverride?: {}) => void; | ||
| warning: (message: string, title: string, optionsOverride?: {}) => void; | ||
| error: (message: any, title: any, optionsOverride?: any) => void; |
There was a problem hiding this comment.
If the intent is a React element, you should use React.ReactNode instead of any.
Is there a reason optionsOverride was changed from a generic object to any?
types/react-toastr/index.d.ts
Outdated
| @@ -1,6 +1,6 @@ | |||
| // Type definitions for react-toastr 3.0 | |||
| // Type definitions for react-toastr 3.1 | |||
There was a problem hiding this comment.
I think the intent is for this version number to be react-toastr's version number, which is still 3.0.0. Someone who maintains DefinitelyTyped can confirm.
|
@DanRegazzi One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you! |
Change toast notify functions to use string or ReactNode types explicitly Revert change to options override type
|
🔔 @shssoichiro - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate? |
types/react-toastr/index.d.ts
Outdated
| info: (message: string, title: string, optionsOverride?: {}) => void; | ||
| success: (message: string, title: string, optionsOverride?: {}) => void; | ||
| warning: (message: string, title: string, optionsOverride?: {}) => void; | ||
| error: (message: string | React.ReactNode, title: string | React.ReactNode, optionsOverride?: {}) => void; |
There was a problem hiding this comment.
ReactNode includes string, so you only need so specify ReactNode.
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/index.d.ts#L148
|
A definition author has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped! |
|
🌟 🎈 🎉 🏆 🎂 ✨ ⭐️ Congratulations on your first DefinitelyTyped contribution! 🌟 🎈 🎉 🏆 🎂 ✨ ⭐️ |
The type definition only allowed strings to be used for the toast title and message content, but react-toastr should also allow JSX elements to be used. Updated the type definition to allow 'any' type for the toast title and message content to allow for strings or JSX Elements.
If changing an existing definition: