fix(alerts provider): make plugin props optional #1384
Conversation
| showAlertsInPlugin, | ||
| children, | ||
| plugin = false, | ||
| parentAlertsAdd = undefined, |
There was a problem hiding this comment.
It's certainly not necessary to provide these default values, not sure what our team thinks is good good practice here?
I like seeing default values, but that's personal / preference.
There was a problem hiding this comment.
Do you also want to add a default false for showAlertsInPlugin?
tomzemp
left a comment
tomzemp
left a comment
There was a problem hiding this comment.
Left some comments for your consideration, but looks good 🙏
| showAlertsInPlugin, | ||
| children, | ||
| plugin = false, | ||
| parentAlertsAdd = undefined, |
There was a problem hiding this comment.
Do you also want to add a default false for showAlertsInPlugin?
| parentAlertsAdd: any | ||
| showAlertsInPlugin: boolean | ||
| children: React.ReactNode | ||
| plugin?: boolean |
There was a problem hiding this comment.
plugin should actually always be defined as long as you are using an up-to-date version of app-platform (https://github.com/dhis2/app-platform/blob/master/shell/src/App.js#L43). I'm not sure if there's a scenario where we want to mark it as an optional in case you're trying to resolve to a higher-version of app-runtime while using an older version of app-platform?
|
While moving the instance manager FE from CRA to Vite, I encountered one type issue in this repository:
The
AlertsProvider's plugin props aren't optional