-
Notifications
You must be signed in to change notification settings - Fork 69
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
Refactoring of class components to functional components #7629
base: develop
Are you sure you want to change the base?
Conversation
Your environment is deployed to https://ocrvs-993.opencrvs.dev. |
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.
I noticed some styling issues in your changes, so I suggest you install prettier
& eslint
plugin in your VSCode (I assumed that's what you're using as an editor) which should take care of those
@@ -17,7 +17,8 @@ import { injectIntl, WrappedComponentProps } from 'react-intl' | |||
import styled, { withTheme } from 'styled-components' | |||
import { CompletenessRateTime } from '@client/views/SysAdmin/Performance/utils' | |||
import { messages } from '@client/i18n/messages/views/performance' | |||
import type { LegendProps } from 'recharts' | |||
// import type { LegendProps } from 'recharts' |
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 can directly remove this if it's not needed anymore
// Not needed for functional component | ||
/*interface IRejectTabState { |
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.
Let's remove it if it's not needed
useEffect(() => { | ||
function recordWindowWidth() { | ||
setWidth(window.innerWidth); | ||
} | ||
window.addEventListener('resize', recordWindowWidth); | ||
return () => window.removeEventListener('resize', recordWindowWidth); | ||
}, []); |
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.
useWindowSize
interface IActiveState { | ||
value: number | ||
stroke: string | ||
} | ||
interface IState { | ||
legendMarginTop: number | ||
legendMarginLeft: number | ||
chartTop: number | ||
chartRight: number | ||
chartBottom: number | ||
chartLeft: number | ||
maximizeXAxisInterval?: boolean | ||
legendLayout: LegendProps['layout'] | ||
activeLabel: string | ||
activeRegisteredInTargetDays: IActiveState | ||
activeTotalRegistered: IActiveState | ||
activeTotalEstimate: IActiveState | ||
} | ||
|
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.
Probably better to keep it and use in useState<IState>(...)
at line no 146
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.
Awesome work @noah-brunate!
@noah-brunate Could you please update the CHANGELOG describing what we achieved on this PR? It should go under 1.7 -> Improvements |
This comment has been minimized.
This comment has been minimized.
@noah-brunate looks like there are failing tests here, you can look at my comment here on how to run the tests locally on your machine: #7665 (comment) |
CHANGELOG.md
Outdated
@@ -11,6 +11,7 @@ | |||
- Fetch child identifier in view record | |||
- Auth token, ip address, remote address redacted from server log | |||
- **Align Patient data model with FHIR**: Previously we were using `string[]` for `Patient.name.family` field instead of `string` as mentioned in the FHIR standard. We've now aligned the field with the standard. | |||
- Changed multiple class components into functional components [#993](https://github.com/opencrvs/opencrvs-core/issues/993) |
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.
- Changed multiple class components into functional components [#993](https://github.com/opencrvs/opencrvs-core/issues/993) | |
- Refactored multiple class components into functional ones to facilitate the usage of `useWindowSize` hook [#993](https://github.com/opencrvs/opencrvs-core/issues/993) |
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.
For such a internal change, we can also omit the changelog mention completely. These types of changes do not really affect country implementors or users of OpenCRVS
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.
noted, but will it not affect the merging if needed as a requirement
@noah-brunate The tests need to be fixed before thie can be merged in |
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.
Tests need to be passing
I have converted the class component in the file RequiresUpdate.tsx into a functional component.