chore(js-ts): Convert app/components/UI/SettingsNotification/index.js to TypeScript #11339
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…faultProps in SettingsNotification
devin-ai-integration
bot
added
No QA Needed
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
| interface SettingsNotificationProps { | ||
| style?: StyleProp<ViewStyle>; | ||
| isWarning?: boolean; | ||
| isNotification?: boolean; | ||
| children?: React.ReactNode; | ||
| } |
There was a problem hiding this comment.
Are all these props optional? It seems from the component body that if isNotification is true then style is required.
It would also seem that children is required as there is no condition check such as { children.length > 0 && children }
Hard to anticipate, I'm aware. However, as a developer, when I come to import and use this component I won't get a TypeScript warning that a prop is missing, which defeats the purpose of using TypeScript to check props that are necessary.
There was a problem hiding this comment.
@Daniel-Cross Thanks for pointing these out. I have made the children prop as necessary. However, style is breaking existing test cases; that's why I made it optional only. Since it won't be required when isNotification is false, this can't be made required straightforwardly in the interface. Maybe we can add extra validation for this when isNotification is true
devin-ai-integration
bot
added
the
skip-sonar-cloud
|
Triggering CI rerun |
|
Link to Devin run: https://preview.devin.ai/devin/b44f5f30e5ac4f9bb3f19ec2b4bf235f
If you have any feedback, you can leave comments in the PR and I'll address them in the app!