Skip to content
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

Closed
wants to merge 21 commits into from

Conversation

peterMuriuki
Copy link
Collaborator

What i think changes using react-Async would like.

@peterMuriuki
Copy link
Collaborator Author

I have not added the tests yet, just needed some eyes on this

@peterMuriuki peterMuriuki requested review from cKellyDesign and moshthepitt and removed request for cKellyDesign April 22, 2020 08:22
@peterMuriuki
Copy link
Collaborator Author

reactAsync

request for comment

@@ -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
Copy link
Contributor

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?

Copy link
Collaborator Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, cool

Copy link
Contributor

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm,...

Copy link
Collaborator Author

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?

Copy link
Contributor

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooh!,.. ok.

Copy link
Contributor

@cKellyDesign cKellyDesign left a 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()?

@coveralls
Copy link

coveralls commented Apr 23, 2020

Coverage Status

Coverage increased (+0.06%) to 76.854% when pulling 871af57 on reactAsyncPractitionerView into 99cd428 on 554-abortFetch.

Comment on lines +98 to +102
{plans.length < 1 ? (
<span>{NO_PLANS_LOADED_MESSAGE}</span>
) : (
<ListView {...listViewProps} />
)}
Copy link
Contributor

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?

Copy link
Collaborator Author

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

Copy link
Contributor

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?

@moshthepitt
Copy link
Contributor

@p-netm should we merge this, or close this?

@moshthepitt
Copy link
Contributor

Going to close this.

@moshthepitt moshthepitt deleted the reactAsyncPractitionerView branch March 22, 2021 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants