-
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
React async practitioner view #822
Conversation
I have not added the tests yet, just needed some eyes on this |
@@ -78,6 +78,7 @@ const IRSAssignmentPlansList = (props: PlanAssignmentsListProps) => { | |||
/** async function to load the data */ | |||
async function loadData() { | |||
try { | |||
setLoading(plans.length < 1); // only set loading when there are no plans |
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.
Why use plans.length < 1
as the test here? iirc we are trying to move away from that, no?
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.
Ooh yeah, this is a stoaway commit when branching of. The changes are only for practitioners list view
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.
Cool, cool
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.
How can we make this so generic that when we implement this when fetching plans, or jurisdictions or whatever else (from OpenSRP for now), we don't have to re-write similar(ish) code?
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.
hmm,...
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.
How can we make this so generic that when we implement this when fetching plans, or jurisdictions or whatever else (from OpenSRP for now), we don't have to re-write similar(ish) code?
@moshthepitt So just to be clear, the quoted text above is about the react-async changes introduced in the practitionerListView and not in the IRS file of which this thread is part of, ama?
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 what you mean, but what I mean is the usage of react-async to combat the problems we identified in #806 should be as DRY as possible across different components.
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.
ooh!,.. ok.
Help keep code dry when using react-async and avoid boilerplate
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 looks really great @p-netm, good work! Just one magic string and one typo then this looks good to me once tests are passing! Also, are there tests for asyncGetPractitioners()
?
src/containers/pages/PractitionerViews/PractitionerListView/index.tsx
Outdated
Show resolved
Hide resolved
{plans.length < 1 ? ( | ||
<span>{NO_PLANS_LOADED_MESSAGE}</span> | ||
) : ( | ||
<ListView {...listViewProps} /> | ||
)} |
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.
Is it possible to do this in the AsyncRenderer
component?
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 would then make the component re-suable only when we want to show a table. Do you think its a reasonable constraint, coz anyway, I'd be happy to refactor the AsyncRenderer
. @moshthepitt
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.
That is a fair point @p-netm - However how do we make it such that we dont end up rewriting the same code in different components that want to display a list of items?
@p-netm should we merge this, or close this? |
Going to close this. |
What i think changes using react-Async would like.