Remove Breadcrumbs
#10322
Remove Breadcrumbs
#10322
Conversation
WalkthroughThe pull request focuses on removing breadcrumb functionality across multiple components in the frontend application. The changes involve deleting the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@rithviknishad, it’s not done yet, but I just want to confirm if this is the right approach ? |
|
yeah |
|
delete the shadcn's breadcrumbs too if they are not used |
the organization layout is using that |
|
I think we should keep the 
We can embed it in Page Component directly |
|
yup |
|
@AdityaJ2305 what is the update on this PR :) |
Just going to push the final commit |
|
Hey Team, I have commented some code snippets just let me know if it is fine or remove them ? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/Common/PageTitle.tsx (1)
54-69: LGTM! Well-implemented back button.The back button implementation is clean and includes:
- Proper conditional rendering
- Correct handling of custom back click behavior
- Appropriate styling and accessibility attributes
Consider adding an aria-label to improve accessibility:
<Button variant="link" type="button" className="rounded bg-gray-200/50 px-1 text-sm font-normal text-gray-800 transition hover:bg-gray-200/75 hover:no-underline" size="xs" + aria-label="Go back to previous page" onClick={() => { if (onBackClick && onBackClick() === false) return; goBack(backUrl); }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/components/Common/Breadcrumbs.tsx(0 hunks)src/components/Common/Page.tsx(0 hunks)src/components/Common/PageTitle.tsx(3 hunks)src/components/Patient/FileUploadPage.tsx(0 hunks)src/components/Resource/ResourceBoard.tsx(0 hunks)src/components/Resource/ResourceCreate.tsx(3 hunks)src/components/Resource/ResourceDetails.tsx(1 hunks)src/components/Resource/ResourceDetailsUpdate.tsx(4 hunks)src/components/Resource/ResourceList.tsx(0 hunks)src/components/Users/UserHome.tsx(0 hunks)src/pages/Appointments/AppointmentDetail.tsx(2 hunks)src/pages/Appointments/AppointmentsPage.tsx(0 hunks)src/pages/Encounters/EncounterShow.tsx(1 hunks)src/pages/Facility/settings/organizations/FacilityOrganizationIndex.tsx(2 hunks)src/pages/Facility/settings/organizations/components/FacilityOrganizationLayout.tsx(1 hunks)src/pages/Organization/components/OrganizationLayout.tsx(1 hunks)
💤 Files with no reviewable changes (7)
- src/pages/Appointments/AppointmentsPage.tsx
- src/components/Common/Page.tsx
- src/components/Users/UserHome.tsx
- src/components/Resource/ResourceBoard.tsx
- src/components/Resource/ResourceList.tsx
- src/components/Patient/FileUploadPage.tsx
- src/components/Common/Breadcrumbs.tsx
✅ Files skipped from review due to trivial changes (3)
- src/pages/Organization/components/OrganizationLayout.tsx
- src/pages/Facility/settings/organizations/FacilityOrganizationIndex.tsx
- src/components/Resource/ResourceCreate.tsx
🔇 Additional comments (7)
src/pages/Facility/settings/organizations/components/FacilityOrganizationLayout.tsx (1)
92-92: Consider removing the custom breadcrumb implementation.The PR's objective is to remove breadcrumb functionality, but this file still contains a custom breadcrumb implementation. However, since these breadcrumbs are used to display the organization hierarchy and provide essential navigation context, we should discuss whether to keep this implementation as an exception.
Let me check if this breadcrumb implementation is used elsewhere in the codebase:
Also applies to: 93-117
✅ Verification successful
Keep the organization hierarchy breadcrumb as an exception
This breadcrumb implementation is specifically designed for navigating the organization hierarchy, showing parent-child relationships and providing essential navigation context. It's consistently used across organization-related layouts and should be retained as an exception to the general breadcrumb removal.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar breadcrumb implementations ast-grep --pattern 'import { Breadcrumb, BreadcrumbItem, BreadcrumbLink, BreadcrumbList, BreadcrumbSeparator } from "@/components/ui/breadcrumb"'Length of output: 1444
Script:
#!/bin/bash # Check the breadcrumb implementation in both files echo "=== FacilityOrganizationLayout.tsx ===" ast-grep --pattern 'const $_ = () => { $$$ return ( $$$ <Breadcrumb$$$> $$$ </Breadcrumb> $$$ ) }' echo -e "\n=== OrganizationLayout.tsx ===" ast-grep --pattern '<Breadcrumb$$$> $$$ </Breadcrumb>'Length of output: 351
Script:
#!/bin/bash echo "=== FacilityOrganizationLayout.tsx ===" rg -A 10 "<Breadcrumb" src/pages/Facility/settings/organizations/components/FacilityOrganizationLayout.tsx echo -e "\n=== OrganizationLayout.tsx ===" rg -A 10 "<Breadcrumb" src/pages/Organization/components/OrganizationLayout.tsxLength of output: 2403
src/pages/Encounters/EncounterShow.tsx (1)
100-100: LGTM!The changes align with the PR objectives by removing breadcrumb functionality while maintaining proper navigation using the backUrl.
src/components/Resource/ResourceDetails.tsx (1)
120-120: LGTM!The changes align with the PR objectives by removing breadcrumb functionality while maintaining proper page structure.
src/pages/Appointments/AppointmentDetail.tsx (1)
60-60: LGTM! Changes align with PR objective.The removal of breadcrumb-related functionality is consistent with the PR's goal. The changes include:
- Removing the
formatAppointmentSlotTimeimport- Commenting out the
appointmentDatevariable- Simplifying the
Pagecomponent's propsAlso applies to: 138-138, 141-141
src/components/Common/PageTitle.tsx (3)
3-9: LGTM! Import changes are well-organized.The new imports are properly organized and provide the necessary components and utilities for the back button functionality.
Also applies to: 12-12
40-40: LGTM! Proper use of navigation hook.The
useAppHistoryhook is correctly used to handle back navigation.
51-51: LGTM! Improved class name management.The switch to the
cnutility function for managing class names is a good improvement, making the code more maintainable.Also applies to: 75-79
| @@ -62,7 +62,7 @@ const initialState = { | |||
|
|
|||
| export const ResourceDetailsUpdate = (props: resourceProps) => { | |||
| const { goBack } = useAppHistory(); | |||
| const [qParams, _] = useQueryParams(); | |||
| // const [qParams, _] = useQueryParams(); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out code.
Instead of keeping the commented-out code, we should remove it since it's no longer needed.
- // const [qParams, _] = useQueryParams();| // const { data: resourceDetails } = useTanStackQueryInstead( | ||
| // routes.getResourceDetails, | ||
| // { | ||
| // pathParams: { id: props.id }, | ||
| // onResponse: ({ res, data }) => { | ||
| // if (res && data) { | ||
| // const d = data; | ||
| // d["status"] = qParams.status || data.status; | ||
| // dispatch({ type: "set_form", form: d }); | ||
| // } | ||
| // setIsLoading(false); | ||
| // }, | ||
| // }, | ||
| // ); |
There was a problem hiding this comment.
Remove commented-out resource details fetching logic.
The commented-out resource details fetching logic should be removed. If this functionality is needed, it should be reimplemented without relying on query parameters.
- // const { data: resourceDetails } = useTanStackQueryInstead(
- // routes.getResourceDetails,
- // {
- // pathParams: { id: props.id },
- // onResponse: ({ res, data }) => {
- // if (res && data) {
- // const d = data;
- // d["status"] = qParams.status || data.status;
- // dispatch({ type: "set_form", form: d });
- // }
- // setIsLoading(false);
- // },
- // },
- // );
+ const { data: resourceDetails } = useTanStackQueryInstead(
+ routes.getResourceDetails,
+ {
+ pathParams: { id: props.id },
+ onResponse: ({ res, data }) => {
+ if (res && data) {
+ dispatch({ type: "set_form", form: data });
+ }
+ setIsLoading(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.
| // const { data: resourceDetails } = useTanStackQueryInstead( | |
| // routes.getResourceDetails, | |
| // { | |
| // pathParams: { id: props.id }, | |
| // onResponse: ({ res, data }) => { | |
| // if (res && data) { | |
| // const d = data; | |
| // d["status"] = qParams.status || data.status; | |
| // dispatch({ type: "set_form", form: d }); | |
| // } | |
| // setIsLoading(false); | |
| // }, | |
| // }, | |
| // ); | |
| const { data: resourceDetails } = useTanStackQueryInstead( | |
| routes.getResourceDetails, | |
| { | |
| pathParams: { id: props.id }, | |
| onResponse: ({ res, data }) => { | |
| if (res && data) { | |
| dispatch({ type: "set_form", form: data }); | |
| } | |
| setIsLoading(false); | |
| }, | |
| }, | |
| ); |
|
In general test, it looks good and cypress passed |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Common/PageTitle.tsx (3)
7-7: Consider removing unused breadcrumb imports.Since the PR's objective is to remove breadcrumbs functionality, importing
BreadcrumbItemseems inconsistent. If it's only being used as a wrapper for styling purposes, consider using a more semantic element or creating a dedicated navigation component.
63-63: Remove commented console.log statement.Remove the commented debug statement to maintain clean code.
- // console.log(backUrl);
56-69: Consider enhancing button accessibility.While the back button implementation is good, consider adding
aria-labelfor better screen reader support.<Button variant="link" type="button" className="rounded bg-gray-200/50 px-1 text-sm font-normal text-gray-800 transition hover:bg-gray-200/75 hover:no-underline" size="xs" + aria-label="Go back to previous page" onClick={() => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/Common/PageTitle.tsx(3 hunks)src/components/Resource/ResourceCreate.tsx(3 hunks)src/components/Resource/ResourceDetails.tsx(1 hunks)src/hooks/useAppHistory.ts(1 hunks)src/pages/Encounters/EncounterShow.tsx(1 hunks)src/pages/Facility/settings/organizations/FacilityOrganizationIndex.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/hooks/useAppHistory.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/pages/Facility/settings/organizations/FacilityOrganizationIndex.tsx
- src/components/Resource/ResourceDetails.tsx
- src/pages/Encounters/EncounterShow.tsx
- src/components/Resource/ResourceCreate.tsx
🔇 Additional comments (3)
src/components/Common/PageTitle.tsx (3)
Line range hint
76-80: LGTM! Well-structured responsive styling.The className conditions are well organized and follow mobile-first responsive design patterns. The use of the
cnutility for class merging is a good practice.
Line range hint
14-24: LGTM! Well-defined props interface.The props interface is well-structured with proper TypeScript types and follows React naming conventions. The removal of breadcrumb-related props and addition of back navigation props aligns well with the PR objectives.
Line range hint
27-89: LGTM! Clean and maintainable implementation.The component is well-structured with:
- Clean separation of concerns
- Proper use of React hooks
- Maintainable implementation
- Good error handling
The changes successfully transition from breadcrumbs to a simpler back navigation pattern as intended by the PR.
|
Change of decision, we need breadcrumbs.... :/ |
|
@rithviknishad , I can make the changes in this PR according to final required behaviour |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Refactor
Bug Fixes
Chores