-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add wizard UI #468
Add wizard UI #468
Conversation
…ion into feat/skeleton-app
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.
Code looking very good, some cleanArch and details:
return apiToFuture(this.dataStore.get<D2Logs[]>(LOGS_PAGE_PREFIX + currentPage)).map( | ||
d2Logs => { | ||
if (!d2Logs) return []; | ||
const logs = d2Logs?.map(d2Log => this.buildLog(d2Log)); |
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.
?
not necessary as d2Logs has been narrowed down
if (!d2Logs) return []; | ||
const logs = d2Logs?.map(d2Log => this.buildLog(d2Log)); | ||
return logs.filter(log => | ||
log.dataSets.some(dataset => options.dataSetsIds?.includes(dataset.id)) |
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.
?
not necessary
|
||
getByDate(options: GetLogsOptions): FutureData<Log[]> { | ||
return this.getCurrentPage().flatMap(currentPage => { | ||
return apiToFuture(this.dataStore.get<D2Logs[]>(LOGS_PAGE_PREFIX + currentPage)).map( |
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.
If there is time, we try to validate data from the dataStore (typically using purify-ts Codec)
|
||
export type D2LegacyAction = { action: Log["action"]; description: string }; | ||
const legacyActionsNames: Record<string, D2LegacyAction> = { | ||
"edit dataset": { action: "edit", description: i18n.t("edit dataset") }, |
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.
i18n.t cannot be used in root level definitions, as the file will be loaded before the language is set. To typically we wrap it in a function. [check the full source]
page: options.paging.page, | ||
filter: { | ||
"attributeValues.attribute.id": { eq: attributeData.id }, | ||
"attributeValues.value": { eq: "true" }, |
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.
Not sure this works as intended. You want to check if exists attribute[id==attributeData.id]=="true"
, but DHIS2 filters are independent so it will be included if the attribute is there and some "true" value is there (not necessarily for that attribute)
types={["Outputs", "Outcomes"]} | ||
themes={[]} | ||
groups={[]} | ||
onClose={() => setShowFilterModal(false)} |
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.
same
}); | ||
|
||
export const FilterTable = React.memo(() => { | ||
const [value, setValue] = React.useState(1); |
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.
can we use more declarative values than 1, 2 or it must be numerical?
); | ||
}); | ||
|
||
ShareOptionsDataSet.displayName = "ShareOptionsDataSet"; |
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 get the idea, we want a nice name in the React Dev Tools. I have not yet come to a conclusion of what's the best way, but in any case we should abstract it in a function so if we change the method we don't need to check every file. Proposal:
const CardGrid: React.FC<CardGridProps> = (props) => { ... };
// Move to its own file
export function component<Props>(comp: React.FC<Props>) {
comp.displayName = comp.name?.replace(/_+$/, "") || "UnknownComponent";
return React.memo(comp);
}
export default component(CardGrid); // method1
export const CardGrid = component(CardGrid_); // method2
What do you think?
: _([...dataSetAccess, ...(selectedValues || [])]) | ||
.uniqBy(access => access.id) | ||
.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.
This logic looks like it's not view-only, so it can be moved to the domain
|
||
{/* Default route */} | ||
<Route path="/dataSets/create" render={() => <RegisterDataSetPage />} /> | ||
<Route path="/dataSets/:id/edit" render={() => <RegisterDataSetPage />} /> |
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.
A recommendation before the routes get bigger. In my experience, it's good to create some router.ts that centralizes the paths (parse/generation) so we don't have history.push(...) all over the app. We still use react-router-dom, of course.
@eperedo I don't see any new commits, what do I have to review? |
Ok, I'll remove myself as reviewer of this PR then. |
📌 References
📝 Implementation
📹 Screenshots/Screen capture
Setup dataSet
Indicators
Share settings
Summary and Save
🔥 Notes to the tester
#86960mqab