-
Notifications
You must be signed in to change notification settings - Fork 477
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
replace useDispatch with useQuery/request in facility module (Part 2, E-Z) (src/Components/Facility/[E-Z]*.tsx #6448
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
✅ Deploy Preview for care-egov-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
❌ Deploy Preview for care-net failed.
|
👋 Hi, @sriharsh05, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
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.
-
Paginated Responses needs to be wrapped with
PaginatedResponse<ModelName>
instead of repeatedly redefining the paginated response's interface. -
Do not create new interfaces when such interfaces are already defined elsewhere in the codebase. Re-use those existing interfaces instead.
-
Avoid using
any
type.
hey @rithviknishad |
@sriharsh05 clear the merge conflict |
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.
@sriharsh05 could you clear the merge conflicts?
👋 Hi, @sriharsh05, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
@rithviknishad |
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 PR is hard to review and test! Too many things can break and would be hard to test.
@sriharsh05 Feel free to make the requested changes in multiple new smaller PR's each targeting 1-3 files max.
export type IInventoryLogResponse = Omit< | ||
IInventorySummaryResponse, | ||
"results" | ||
> & { | ||
results: { |
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.
You are omitting results and adding results of different type. Define a new type instead since this approach will simply make the code hard to read and hard to maintain
interface FacilityObjectModel { | ||
id: string; | ||
name: string; | ||
local_body: number; | ||
district: number; | ||
state: number; | ||
ward_object: WardModel; | ||
local_body_object: { id: number } & LocalBodyModel; | ||
district_object: DistrictModel; | ||
state_object: StateModel; | ||
facility_type: string; | ||
read_cover_image_url: string; | ||
features: number[]; | ||
patient_count: string; | ||
bed_count: string; | ||
} |
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 redefine FacilityModel
in so many different ways? Just use the one already defined FacilityModel
right?
location_object: { | ||
id: string; | ||
facility: { | ||
id: string; | ||
name: string; | ||
}; | ||
created_date: string; | ||
modified_date: string; | ||
name: string; | ||
description: string; | ||
location_type: number; | ||
}; |
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.
Map this to use LocationModel instead
location_object: { | |
id: string; | |
facility: { | |
id: string; | |
name: string; | |
}; | |
created_date: string; | |
modified_date: string; | |
name: string; | |
description: string; | |
location_type: number; | |
}; | |
location_object: LocationModel; |
location_object: { | ||
id: string; | ||
facility: { | ||
id: string; | ||
name: string; | ||
}; | ||
created_date: string; | ||
modified_date: string; | ||
name: string; | ||
description: string; | ||
location_type: number; | ||
}; |
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.
Use LocationModel instead
location_object: { | |
id: string; | |
facility: { | |
id: string; | |
name: string; | |
}; | |
created_date: string; | |
modified_date: string; | |
name: string; | |
description: string; | |
location_type: number; | |
}; | |
location_object: LocationModel; |
last_service: { | ||
id: string; | ||
edits: ServiceEdit[]; | ||
external_id: string; | ||
created_date: string; | ||
modified_date: string; | ||
serviced_on: string; | ||
note: string; | ||
}; |
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 should point to an already defined model instead. Avoid redefing types again and again.
👋 Hi, @sriharsh05, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
Closing as smaller PRs have been made for the same |
WHAT
🤖 Generated by Copilot at 39dcf7c
No summary available (Limit exceeded: required to process 63082 tokens, but only 50000 are allowed per call)
The pull request partially refactors the codebase for managing facility module in the frontend. It replaces the redux actions and states with the custom request function and the useQuery hook for data fetching and updating. It improves the code quality, performance, and readability by removing unnecessary imports, variables, and dependencies.
Proposed Changes
useDispatch
w.useQuery
/request
: Facility (Part 2, E-H) (src/Components/Facility/[E-H]*.tsx
) #6391@coronasafe/care-fe-code-reviewers @coronasafe/code-reviewers
Merge Checklist
HOW
🤖 Generated by Copilot at 39dcf7c
No walkthrough available (Limit exceeded: required to process 63082 tokens, but only 50000 are allowed per call)