Update for List View in Shifting from Gird LayOut to List Layout #8882
Conversation
❌ Deploy Preview for care-ohc failed.
|
AnveshNalimela
changed the title
|
👋 Hi, @AnveshNalimela, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
If we are making this change, I would really make it a list ie like a table; than have a grid; 
|
|
Any Changes required if yes. Suggest |
|
@AnveshNalimela Please use the reference UI shared to update the list view. |
|
|
AnveshNalimela
changed the title
|
|
WalkthroughThe pull request introduces significant modifications to the Changes
Assessment against linked issues
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/Locale/ta.json (1)
Line range hint
1-812: Standardize variable placeholder usageThere are inconsistencies in how variable placeholders are used. For example:
{{name}}is used in some places{{type}}is used differently in others{{version}}appears without consistent formattingConsider standardizing all variable placeholders to use the double curly brace format consistently.
src/components/Shifting/ShiftingList.tsx (3)
160-160: Simplify property access using optional chainingYou can simplify null-safe property access by using optional chaining:
- {(shift.origin_facility_object || {}).name} + {shift.origin_facility_object?.name}🧰 Tools
🪛 Biome
[error] 160-160: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
171-171: Simplify property access using optional chainingSimilarly, you can update this expression for better readability:
- {(shift.shifting_approving_facility_object || {}).name} + {shift.shifting_approving_facility_object?.name}🧰 Tools
🪛 Biome
[error] 171-171: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
228-231: Remove commented-out code for cleaner codebaseIt's good practice to remove unused code to maintain clarity:
- // show={modalFor === shift.external_id} - // onClose={() => - // setModalFor({ externalId: undefined, loading: false }) - // }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/Locale/en.json(1 hunks)src/Locale/hi.json(1 hunks)src/Locale/kn.json(1 hunks)src/Locale/ml.json(1 hunks)src/Locale/ta.json(1 hunks)src/components/Resource/ResourceBoard.tsx(1 hunks)src/components/Shifting/ShiftingList.tsx(6 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/Locale/kn.json
- src/Locale/ml.json
- src/components/Resource/ResourceBoard.tsx
🧰 Additional context used
🪛 Biome
src/components/Shifting/ShiftingList.tsx
[error] 160-160: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 171-171: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
src/Locale/hi.json (3)
Line range hint 1-812: JSON structure looks good!
The file follows proper JSON formatting with consistent key-value pairs.
Line range hint 1-812: Special character encoding is properly handled!
All Hindi characters and special characters are correctly encoded in the JSON file.
Line range hint 1-812: Verify alignment with translation policy.
Based on previous review comments ("skip adding translations for other languages"), please confirm if adding this complete Hindi translation file aligns with the current translation policy.
src/Locale/ta.json (2)
Line range hint 1-812: File structure follows JSON standards correctly
The JSON structure is well-formatted with consistent key-value pairs and proper nesting.
Line range hint 1-812: Good translation approach for technical terms
Technical acronyms (PRN, ABDM, CSV etc.) are appropriately kept in English while surrounding context is translated to Tamil, maintaining clarity for users.
src/Locale/en.json (1)
477-477: LGTM! The new localization entry follows conventions.
The added key-value pair "contact_info": "Contact Info" is well-formatted, follows naming conventions, and is properly placed in alphabetical order.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…la/care_fe into issue/8832/ui-align
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/Shifting/ShiftingList.tsx (2)
161-162: Use optional chaining for safer property access.Replace the object destructuring with optional chaining to handle potential undefined values more elegantly.
-{(shift.origin_facility_object || {}).name} +{shift.origin_facility_object?.name} -{(shift.shifting_approving_facility_object || {}).name} +{shift.shifting_approving_facility_object?.name}Also applies to: 172-173
🧰 Tools
🪛 Biome
[error] 161-161: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
94-104: Enhance accessibility for contact information.The contact information section could benefit from improved accessibility attributes.
<div className="phone number"> <dt title={t("phone_number")} + aria-label={t("phone_number")} className="flex items-center text-sm font-medium leading-5 text-secondary-500" > <CareIcon icon="l-mobile-android" className="mr-2" /> - <dd className="text-sm font-bold leading-5 text-secondary-900"> + <dd className="text-sm font-bold leading-5 text-secondary-900" aria-live="polite"> {shift.patient_object.phone_number || ""} </dd> </dt> </div>Also applies to: 105-115
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/Locale/ta.json(1 hunks)src/components/Shifting/ShiftingList.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Locale/ta.json
🧰 Additional context used
🪛 Biome
src/components/Shifting/ShiftingList.tsx
[error] 161-161: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 172-172: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
src/components/Shifting/ShiftingList.tsx (2)
307-323: Well-implemented responsive design for list view.
The responsive grid layout with conditional rendering for different screen sizes is well-implemented. The headers are appropriately hidden on mobile views and the layout adjusts nicely between different screen sizes.
83-237: Successfully implemented list view with improved UI.
The implementation effectively addresses the PR objectives:
- Shifted from grid to list layout with proper alignment
- Enhanced visibility of shifting status and actions
- Improved component organization with clear visual hierarchy
- Responsive design that works well across different screen sizes
The code is well-structured and maintainable.
🧰 Tools
🪛 Biome
[error] 161-161: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 172-172: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/components/Shifting/ShiftingList.tsx (3)
34-40: LGTM! Consider extracting the initial state.The state type definition is well-focused. Consider extracting the initial state to a constant for better maintainability.
+const INITIAL_MODAL_STATE = { + externalId: undefined, + loading: false, +} as const; -const [modalFor, setModalFor] = useState<{ - externalId: string | undefined; - loading: boolean; -}>({ - externalId: undefined, - loading: false, -}); +const [modalFor, setModalFor] = useState(INITIAL_MODAL_STATE);
161-161: Use optional chaining for better null safety.The static analysis correctly identified opportunities to use optional chaining for facility object properties.
-{(shift.origin_facility_object || {}).name} +{shift.origin_facility_object?.name} -{(shift.shifting_approving_facility_object || {}).name} +{shift.shifting_approving_facility_object?.name}Also applies to: 172-172
🧰 Tools
🪛 Biome
[error] 161-161: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
229-232: Remove commented code.Remove the commented-out code as it's no longer needed and can cause confusion.
- // show={modalFor === shift.external_id} - // onClose={() => - // setModalFor({ externalId: undefined, loading: false }) - // }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/Locale/ta.json(1 hunks)src/components/Shifting/ShiftingList.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Locale/ta.json
🧰 Additional context used
🪛 Biome
src/components/Shifting/ShiftingList.tsx
[error] 161-161: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 172-172: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
src/components/Shifting/ShiftingList.tsx (1)
307-323: LGTM! Well-structured responsive layout.
The grid layout with responsive classes effectively handles different screen sizes. The mobile-first approach with hidden elements on smaller screens improves the user experience.
|
@nihal467 @rithviknishad Made the changes mentioned and Updated PR and screenshots |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
src/components/Shifting/ShiftingList.tsx (5)
Line range hint
1-31: Consider organizing imports by category.Consider grouping imports into categories for better maintainability:
- React and hooks
- Components
- Utils and configs
- Types and models
Line range hint
45-53: Add error handling to transfer completion.Consider adding try-catch block to handle potential errors during transfer completion and show appropriate error messages to users.
const handleTransferComplete = async (shift: ShiftingModel) => { setModalFor({ ...modalFor, loading: true }); - await request(routes.completeTransfer, { - pathParams: { externalId: shift.external_id }, - }); - navigate( - `/facility/${shift.assigned_facility}/patient/${shift.patient}/consultation`, - ); + try { + await request(routes.completeTransfer, { + pathParams: { externalId: shift.external_id }, + }); + navigate( + `/facility/${shift.assigned_facility}/patient/${shift.patient}/consultation`, + ); + } catch (error) { + // Show error notification + setModalFor({ externalId: undefined, loading: false }); + } };
161-162: Use optional chaining for safer object access.Following the static analysis suggestion, use optional chaining for safer object access:
- {(shift.origin_facility_object || {}).name} + {shift.origin_facility_object?.name} - {(shift.shifting_approving_facility_object || {}).name} + {shift.shifting_approving_facility_object?.name}Also applies to: 172-173
🧰 Tools
🪛 Biome
[error] 161-161: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
83-237: Consider extracting ShiftCard component.The shift card rendering logic is complex and could benefit from being extracted into a separate component for better maintainability and reusability.
Example structure:
interface ShiftCardProps { shift: ShiftingModel; onViewDetails: (externalId: string) => void; onTransferComplete: (shift: ShiftingModel) => void; modalFor: { externalId: string | undefined; loading: boolean }; } const ShiftCard: React.FC<ShiftCardProps> = ({ shift, onViewDetails, onTransferComplete, modalFor }) => { // Current rendering logic };🧰 Tools
🪛 Biome
[error] 161-161: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 172-172: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
307-323: Consider accessibility improvements for responsive layout.While the responsive design looks good, consider:
- Adding
aria-hidden="true"to elements that are hidden on mobile- Ensuring sufficient color contrast for text elements
- Adding screen reader descriptions for icons
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Shifting/ShiftingList.tsx(6 hunks)
🧰 Additional context used
🪛 Biome
src/components/Shifting/ShiftingList.tsx
[error] 161-161: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 172-172: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
src/components/Shifting/ShiftingList.tsx (1)
33-40: LGTM! Good improvement to state management.
The addition of proper typing for modalFor state and inclusion of loading state improves type safety and user experience.
|
👋 Hi, @AnveshNalimela, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
@AnveshNalimela can you clear all the merge conflict to get it ready for review |
AnveshNalimela
force-pushed
the
issue/8832/ui-align
branch
from
3315f2c
to
2cd8c0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/components/Shifting/ShiftingList.tsx (4)
Line range hint
50-58: Add error handling to transfer completion.While the type safety is good, consider adding error handling:
const handleTransferComplete = async (shift: ShiftingModel) => { setModalFor({ ...modalFor, loading: true }); - await request(routes.completeTransfer, { - pathParams: { externalId: shift.external_id }, - }); - navigate( - `/facility/${shift.assigned_facility}/patient/${shift.patient}/consultation`, - ); + try { + await request(routes.completeTransfer, { + pathParams: { externalId: shift.external_id }, + }); + navigate( + `/facility/${shift.assigned_facility}/patient/${shift.patient}/consultation`, + ); + } catch (error) { + // Handle error appropriately + setModalFor({ ...modalFor, loading: false }); + } };
83-245: Consider breaking down the complex list item into smaller components.The current implementation has a lot of nested JSX which makes it harder to maintain. Consider extracting the following into separate components:
- PatientInfo (lines 89-96)
- ContactInfo (lines 98-121)
- ShiftingStatus (lines 123-157)
- FacilityInfo (lines 159-193)
- ActionButtons (lines 194-242)
This would improve:
- Code readability
- Maintainability
- Reusability
- Testing
🧰 Tools
🪛 Biome
[error] 166-166: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 177-177: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
312-328: Enhance accessibility and simplify responsive classes.
- Add ARIA labels for better screen reader support:
-<div className="col-span-1 uppercase sm:text-center md:text-center lg:block lg:text-left"> +<div className="col-span-1 uppercase sm:text-center md:text-center lg:block lg:text-left" role="columnheader" aria-label={t("patients")}>
- Consider using Tailwind's responsive modifiers more efficiently:
-<div className="col-span-1 hidden text-left uppercase sm:hidden md:hidden lg:block"> +<div className="col-span-1 hidden lg:block text-left uppercase">
268-276: Improve search input accessibility and responsiveness.The search input container could benefit from better spacing and accessibility:
<div className="mt-2 flex w-full flex-col items-center justify-between gap-2 pt-2 xl:flex-row"> <SearchInput name="patient_name" value={qParams.patient_name} onChange={(e) => updateQuery({ [e.name]: e.value })} placeholder={t("search_patient")} + aria-label={t("search_patient")} + className="w-full xl:w-96" /> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.env(1 hunks)src/components/Shifting/ShiftingList.tsx(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- .env
🧰 Additional context used
🪛 Biome
src/components/Shifting/ShiftingList.tsx
[error] 166-166: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 177-177: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
src/components/Shifting/ShiftingList.tsx (1)
38-45: Well-structured state management!
Good job on improving type safety and state management by using a proper TypeScript interface and adding loading state for async operations.
| {(shift.origin_facility_object || {}).name} | ||
| </dd> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use optional chaining for safer property access.
As suggested by static analysis, use optional chaining for safer property access:
-{(shift.origin_facility_object || {}).name}
+{shift.origin_facility_object?.name}
-{(shift.shifting_approving_facility_object || {}).name}
+{shift.shifting_approving_facility_object?.name}Also applies to: 177-178
🧰 Tools
🪛 Biome
[error] 166-166: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
|
@Nihal The deploy failed due to cache issue of apps folder.the build got failed due to import statements. Could on the issue @rithviknishad |
Proposed Changes
-Aligned the components in shifitng and resource page
-Increased the search bar size in list view
-Adjusted the componets
-Changed the font to semibold in Resource card
-Changed the gridlayout from 2 cols to 3 cols to remove extra white space
@ohcnetwork/care-fe-code-reviewers
Before:
After:
Story points Requesting: 2 points
Merge Checklist
Summary by CodeRabbit
New Features
ListViewcomponent to display detailed patient information, shifting status, and facility details."contact_info": "Contact Info".Bug Fixes