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.

Sign up for GitHub

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

Fix Auto-Scroll Issue and Total Count Card in facility details page #9275

Closed
wants to merge 5 commits into from
+26 −17
Closed

Fix Auto-Scroll Issue and Total Count Card in facility details page #9275

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2812e9c
Fix Auto-Scroll Issue and Total Count Card in facility details page
shauryag2002 Dec 2, 2024
4d0a553
resolved coderabbitai comments
shauryag2002 Dec 2, 2024
4fb718c
Fix setTotalDoctors in FacilityStaffList
shauryag2002 Dec 7, 2024
48b365c
Refactor DoctorPaginatedResponse to use PaginatedResponse
shauryag2002 Dec 7, 2024
23d3b67
Merge branch 'develop' of https://github.com/shauryag2002/care_fe int…
shauryag2002 Dec 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Utils/request/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,5 @@ export interface PaginatedResponse<TItem> {
next: string | null;
previous: string | null;
results: TItem[];
total_doctors: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider moving domain-specific field to a separate interface

Adding total_doctors to the generic PaginatedResponse interface violates the Interface Segregation Principle, as not all paginated responses will need this field. Consider creating a specific interface for doctor-related responses:

interface DoctorPaginatedResponse<T> extends PaginatedResponse<T> {
  total_doctors: number;
}

}
14 changes: 9 additions & 5 deletions src/components/Common/Pagination.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@ interface PaginationProps {
defaultPerPage: number;
cPage: number;
className?: string;
ScrollToTop?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Follow TypeScript naming conventions

The prop name ScrollToTop should follow camelCase convention as per TypeScript standards.

- ScrollToTop?: boolean;
+ scrollToTop?: boolean;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
ScrollToTop?: boolean;
scrollToTop?: boolean;

}
const Pagination = ({
className = "mx-auto my-4",
data,
onChange,
defaultPerPage,
cPage,
ScrollToTop = true,
}: PaginationProps) => {
const [rowsPerPage, setRowsPerPage] = useState(3);
const [currentPage, setCurrentPage] = useState(1);
Expand Down Expand Up @@ -81,11 +83,13 @@ const Pagination = ({
setCurrentPage(page);
onChange(page, rowsPerPage);
const pageContainer = window.document.getElementById("pages");
pageContainer?.scroll({
top: 0,
left: 0,
behavior: "smooth",
});
if (ScrollToTop) {
pageContainer?.scroll({
top: 0,
left: 0,
behavior: "smooth",
});
}
};

return (
Expand Down
24 changes: 15 additions & 9 deletions src/components/Facility/FacilityStaffList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,19 @@ export const FacilityStaffList = (props: any) => {
},
onResponse: ({ res, data }) => {
if (res?.ok && data) {
let totalCount = 0;
data.results.map((doctor: DoctorModal) => {
if (doctor.count) {
totalCount += doctor.count;
}
});
setTotalDoctors(totalCount);
setTotalDoctors(data?.total_doctors ?? 0);
}
},
});

const handlePageChange = (page: number) => {
updatePage(page);
const staffCapacityElement = document.getElementById("staff_capacity");
if (staffCapacityElement) {
staffCapacityElement.scrollIntoView({ behavior: "smooth" });
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add null check before scrollIntoView

The current implementation might throw an error if the element is not found. Consider adding a null check:

  const handlePageChange = (page: number) => {
    updatePage(page);
    const staffCapacityElement = document.getElementById("staff_capacity");
-   if (staffCapacityElement) {
-     staffCapacityElement.scrollIntoView({ behavior: "smooth" });
-   }
+   staffCapacityElement?.scrollIntoView({ behavior: "smooth" });
  };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const handlePageChange = (page: number) => {
updatePage(page);
const staffCapacityElement = document.getElementById("staff_capacity");
if (staffCapacityElement) {
staffCapacityElement.scrollIntoView({ behavior: "smooth" });
}
};
const handlePageChange = (page: number) => {
updatePage(page);
const staffCapacityElement = document.getElementById("staff_capacity");
staffCapacityElement?.scrollIntoView({ behavior: "smooth" });
};


let doctorList: any = null;
if (!doctorsList || !doctorsList.results.length) {
doctorList = (
Expand Down Expand Up @@ -89,7 +91,10 @@ export const FacilityStaffList = (props: any) => {

return (
<section id="facility-doctor-capacity-details">
<div className="mt-5 rounded bg-white p-3 shadow-sm md:p-6">
<div
className="mt-5 rounded bg-white p-3 shadow-sm md:p-6"
id="staff_capacity"
>
<div className="justify-between md:flex md:pb-2">
<div className="mb-2 text-xl font-bold">Staff Capacity</div>
<ButtonV2
Expand Down Expand Up @@ -128,7 +133,8 @@ export const FacilityStaffList = (props: any) => {
cPage={qParams.page}
defaultPerPage={resultsPerPage}
data={{ totalCount: doctorsList?.count ?? 0 }}
onChange={(page) => updatePage(page)}
ScrollToTop={false}
onChange={(page: number) => handlePageChange(page)}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Maintain consistent prop naming

The ScrollToTop prop should match the casing used in the Pagination component:

- ScrollToTop={false}
+ scrollToTop={false}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
ScrollToTop={false}
onChange={(page: number) => handlePageChange(page)}
scrollToTop={false}
onChange={(page: number) => handlePageChange(page)}

/>
</section>
);
Expand Down
Loading