-
Notifications
You must be signed in to change notification settings - Fork 4
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
MDA Lite Reporting #1536
MDA Lite Reporting #1536
Conversation
… supervisor reporting page
@@ -288,6 +292,20 @@ export class HeaderComponent extends React.Component<HeaderProps, State> { | |||
</DropdownItem> | |||
</div> | |||
)} | |||
|
|||
{ENABLE_MDA_LITE && ( |
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.
Does the enable/disable also work at the route level? i.e Can you access the route if the menu link is disabled?
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.
Yeah the link will be accessible. Fixing that.
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.
@kelvin-muchiri this is affecting all routes in reveal and have created an issue here to address it.
/** columns for mda Lite jurisdictions */ | ||
export const genderReportColumns = [ | ||
{ | ||
Header: 'Male', |
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.
Do we need to have this translated? plus the rest of the table columns
return <Loading />; | ||
} | ||
return ( | ||
<div> |
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.
Would it be better to use React fragment <>...</>
in this particular case instead of <div>....</div>
const tableProps = getCddTableProps(cddReportColumns, cddData, props); | ||
|
||
if (loading) { | ||
return <Loading />; |
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.
Shouldn't we have this piece of code above the code that precedes it. i.e Why should we execute code then check for the loading status? In a case where loading is false, all that execution would be futile
const url = `${REPORT_MDA_LITE_CDD_REPORT_URL}/${original.plan_id}/${original.base_entity_id}/${original.id}`; | ||
return <Link to={url}>{cell.value}</Link>; | ||
}, | ||
Header: 'Name', |
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.
Do we need to translate this table column?
/** CDD columns */ | ||
export const cddReportColumns = [ | ||
{ | ||
Header: 'Name', |
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.
Do we need to translate these table columns?
// table props | ||
const tableProps = getCddTableProps(supervisorColumns, supervisorData, props); | ||
|
||
if (loading) { |
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.
Shouldn't we have this piece of code above the code that precedes it. i.e Why should we execute code then check for the loading status? In a case where loading is false, all that execution would be futile
props: GenericJurisdictionProps & RouteComponentProps<RouteParams> | ||
) => { | ||
return ( | ||
<div> |
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.
Would it be better to use react fragment instead of div?
const url = `${REPORT_MDA_LITE_CDD_REPORT_URL}/${original.plan_id}/${original.base_entity_id}`; | ||
return <Link to={url}>{cell.value}</Link>; | ||
}, | ||
Header: 'Name', |
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.
do we need to translate the column name?
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.
LGTM
reducerRegistry.register(genericPlanReducerName, GenericPlansReducer); | ||
reducerRegistry.register(genericStructuresReducerName, genericStructuresReducer); | ||
|
||
/** selectors */ |
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.
hanging comment.
Object { | ||
"fetchPlans": [Function], | ||
"history": Object { |
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 more of the opinion that we should use equality assertions for things like prop checks as opposed to snapshots.
#1554 |
Part of #1494
Adds following views: