-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature: Performance overview table with indicator linked #15
Feature: Performance overview table with indicator linked #15
Conversation
…ithub.com:EyeSeeTea/zebra-dev into feature/performace-overview-table-link-indicators
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.
thanks @delemaf Some changes requested
|
||
export type DiseaseTotalAttrs = DiseaseEntry | HazardEntry; | ||
|
||
export class AnalyticsD2Repository implements AnalyticsRepository { |
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.
analytics, indicators etc are DHIS concepts. As per clean architecture the repository should only deal with domain entities. So you can you rename the class, file and functions to not have any references to DHIS concepts but instead refer to Zebra concepts - performance overview table, performance overview repository etc.
import { FutureData } from "../../api-futures"; | ||
import { DataStoreClient } from "../../DataStoreClient"; | ||
|
||
export enum ProgramIndicatorsDatastoreKey { |
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.
again, the enums and types should not refer to DHIS concepts like ProgramIndicators, please rename.
@@ -0,0 +1,221 @@ | |||
export enum IndicatorsId { | |||
suspectedDisease = "jLvbkuvPdZ6", |
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 am guessing that these will be fetched from datastore in upcoming PRs?
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.
Yes!
(value: string) => | ||
setOrder && | ||
setOrder({ | ||
name: value, |
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.
whenever your set of a react state depends on its previous state, you need to use an updater function. https://react.dev/reference/react/useState#updating-state-based-on-the-previous-state.
useEffect(() => { | ||
if (dataPerformanceOverview) { | ||
setDataPerformanceOverview(newDataPerformanceOverview => | ||
_(newDataPerformanceOverview) |
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.
whenever your set of a react state depends on its previous state, you need to use an updater function. https://react.dev/reference/react/useState#updating-state-based-on-the-previous-state. Then add dataPerformanceOverviewdata as a dependency to the useEffect
📌 References
📝 Implementation
Needs EyeSeeTea/d2-api#150 to be merged before (it fixes ts error)
📹 Screenshots/Screen capture
🔥 Notes to the tester