Questionnaire and valueset list view enhancement #10712
Conversation
…view and improve layout
WalkthroughThe pull request updates import paths in the AppRouter by replacing relative paths with absolute ones. It enhances two UI components—QuestionnaireList and ValueSetList—by adding responsive rendering methods (RenderCard and RenderTable) to display content appropriately on mobile and desktop views. Additionally, the MedicationRequestTable component’s ScrollArea is modified to use a flexible height based on its content instead of a calculated viewport height. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as Component
participant SC as Screen Checker
participant RC as RenderCard
participant RT as RenderTable
U->>C: Navigate to list view
C->>SC: Check screen size
alt Mobile view (small screen)
SC-->>C: Small screen detected
C->>RC: Invoke RenderCard
RC-->>C: Return card layout
else Desktop view (large screen)
SC-->>C: Large screen detected
C->>RT: Invoke RenderTable
RT-->>C: Return table layout
end
C-->>U: Display rendered UI
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Questionnaire/QuestionnaireList.tsx (1)
166-180: Consider adding a loading state for pagination.While the initial loading state is handled, consider adding a loading state for pagination updates to improve user experience.
<div className="container mx-auto px-4 py-6"> <div className="mb-6 flex items-center justify-between"> {/* ... */} </div> <RenderTable /> <RenderCard /> - <Pagination totalCount={response?.count ?? 0} /> + {isLoading ? ( + <div className="flex justify-center py-4"> + <Loading /> + </div> + ) : ( + <Pagination totalCount={response?.count ?? 0} /> + )} </div>src/components/ValueSet/ValueSetList.tsx (1)
88-102: Consider extracting the edit button to a separate component.The edit button logic appears in both card and table views. Consider extracting it to a reusable component to maintain consistency and reduce duplication.
// src/components/ValueSet/ValueSetEditButton.tsx interface ValueSetEditButtonProps { slug: string; variant?: "ghost" | "default"; size?: "sm" | "default"; onClick?: (e: React.MouseEvent) => void; } export function ValueSetEditButton({ slug, variant = "ghost", size = "sm", onClick }: ValueSetEditButtonProps) { const { t } = useTranslation(); const navigate = useNavigate(); const handleClick = (e: React.MouseEvent) => { if (onClick) { onClick(e); } else { navigate(`/admin/valuesets/${slug}/edit`); } }; return ( <Button variant={variant} size={size} onClick={handleClick} className="hover:bg-primary/5" > {t("edit")} </Button> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Routers/AppRouter.tsx(2 hunks)src/components/Questionnaire/QuestionnaireList.tsx(3 hunks)src/components/ValueSet/ValueSetList.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (6)
src/Routers/AppRouter.tsx (2)
18-23: LGTM! Import paths updated correctly.The import paths for
OrganizationRoutesandAdminRouteshave been updated to use the@alias, which is consistent with other imports in the file.
108-108: Verify scroll behavior across different viewport sizes.The change from
overflow-y-autotooverflow-hiddenremoves the extra scrollbar, but we should ensure this doesn't cause content to be cut off in any viewport size.Please test the following scenarios:
- Very long content that exceeds viewport height
- Different screen sizes (mobile, tablet, desktop)
- Different zoom levels
- Content with dynamic height changes
src/components/Questionnaire/QuestionnaireList.tsx (2)
38-97: LGTM! Well-structured responsive card layout.The card layout for smaller screens is well-implemented with:
- Proper spacing and padding
- Semantic HTML structure
- Accessible text labels
- Clear visual hierarchy
99-164: LGTM! Table layout with proper overflow handling.The table implementation for larger screens correctly handles:
- Overflow with
overflow-hidden- Responsive visibility
- Consistent styling with the card layout
src/components/ValueSet/ValueSetList.tsx (2)
29-112: LGTM! Well-implemented card layout with proper event handling.The card implementation correctly:
- Handles click events with proper event propagation
- Implements consistent styling
- Provides clear visual hierarchy
114-196: LGTM! Table layout with proper accessibility.The table implementation:
- Uses semantic HTML
- Includes proper headers
- Handles empty states correctly
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/ValueSet/ValueSetList.tsx (1)
37-203: 🛠️ Refactor suggestionConsider creating shared components to reduce duplication.
The ValueSetList and QuestionnaireList components share very similar patterns. Consider:
- Creating a generic ListLayout component
- Implementing shared Card and Table components
- Using composition for specific features
Example shared component structure:
// components/Common/ListLayout.tsx interface ListLayoutProps<T> { items: T[]; renderCard: (item: T) => React.ReactNode; renderTable: (items: T[]) => React.ReactNode; title: string; description: string; createButton: React.ReactNode; } export function ListLayout<T>({ items, renderCard, renderTable, title, description, createButton, }: ListLayoutProps<T>) { return ( <div className="container mx-auto px-4 py-6"> {/* Header */} <div className="mb-6 flex items-center justify-between"> <div> <h1 className="text-2xl font-bold">{title}</h1> <p className="text-gray-600">{description}</p> </div> {createButton} </div> {/* Content */} <div className="hidden xl:block">{renderTable(items)}</div> <div className="xl:hidden">{items.map(renderCard)}</div> </div> ); }
🧹 Nitpick comments (4)
src/components/Medicine/MedicationRequestTable/index.tsx (1)
185-188: Consider adding min-height to maintain consistent layout.While the loading state has a
min-h-[200px], the ScrollArea lacks a similar constraint. This could cause layout shifts when transitioning between states.Apply this diff to maintain consistent layout height:
-<ScrollArea className="h-fit"> +<ScrollArea className="h-fit min-h-[200px]">Also applies to: 194-194
src/components/Questionnaire/QuestionnaireList.tsx (2)
46-172: Consider extracting components and reducing duplication.While the responsive implementation is good, consider these improvements:
- Extract RenderCard and RenderTable into separate components
- Create constants for commonly used className strings
- Create a shared click handler
Example refactor:
+ const CARD_CLASSES = "overflow-hidden bg-white rounded-lg cursor-pointer transition-shadow transform hover:shadow-lg"; + const handleQuestionnaireClick = (slug: string) => () => navigate(`/admin/questionnaire/${slug}`); + + const QuestionnaireCard = ({ questionnaire }: { questionnaire: QuestionnaireDetail }) => ( + <Card + key={questionnaire.id} + className={CARD_CLASSES} + onClick={handleQuestionnaireClick(questionnaire.slug)} + > + {/* Card content */} + </Card> + ); + + const QuestionnaireTable = ({ questionnaires }: { questionnaires: QuestionnaireDetail[] }) => ( + <div className="hidden xl:block overflow-hidden rounded-lg bg-white shadow"> + {/* Table content */} + </div> + );
174-189: Enhance accessibility with ARIA attributes and keyboard navigation.Consider adding:
- ARIA labels for interactive elements
- Keyboard navigation support
- Screen reader announcements for dynamic content
Example improvements:
<div className="container mx-auto px-4 py-6"> - <div className="mb-6 flex items-center justify-between"> + <div className="mb-6 flex items-center justify-between" role="banner"> <div> - <h1 className="text-2xl font-bold">{t("questionnaires")}</h1> + <h1 className="text-2xl font-bold" id="page-title">{t("questionnaires")}</h1> <p className="text-gray-600">{t("manage_and_view_questionnaires")}</p> </div> - <Button onClick={() => navigate("/admin/questionnaire/create")}> + <Button + onClick={() => navigate("/admin/questionnaire/create")} + aria-label={t("create_new_questionnaire")} + > {t("create_new")} </Button> </div> - <RenderTable /> - <RenderCard /> + <div role="region" aria-labelledby="page-title"> + <RenderTable /> + <RenderCard /> + </div> <Pagination totalCount={response?.count ?? 0} /> </div>src/components/ValueSet/ValueSetList.tsx (1)
212-217: Enhance create button accessibility.The button implementation is good, but could benefit from additional accessibility attributes.
-<Button asChild> +<Button + asChild + aria-label={t("create_new_valueset")} +> <Link href="/admin/valuesets/create"> <PlusIcon className="mr-2 size-4" /> {t("create_new")} </Link> </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Medicine/MedicationRequestTable/index.tsx(1 hunks)src/components/Questionnaire/QuestionnaireList.tsx(3 hunks)src/components/ValueSet/ValueSetList.tsx(4 hunks)src/pages/Facility/overview.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/pages/Facility/overview.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (2)
src/components/Medicine/MedicationRequestTable/index.tsx (1)
194-194: LGTM! Height change aligns with PR objectives.The change from a viewport-based height to
h-fiteffectively removes the extra scroll bar, allowing the content to determine its natural height. This change, combined with the main layout'soverflow-hidden, ensures a cleaner scrolling experience.src/components/Questionnaire/QuestionnaireList.tsx (1)
1-24: Well-organized imports and component setup!The code follows best practices by:
- Using modern React query patterns with tanstack-query
- Implementing internationalization with i18next
- Utilizing shadcn-ui components for consistent UI
|
Wait for #10678 to merged |
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Questionnaire/QuestionnaireList.tsx (1)
147-148:⚠️ Potential issueFix conflicting overflow classes causing the extra scroll bar.
The table container has both
overflow-hiddenandoverflow-x-autowhich can cause the extra scroll bar issue mentioned in the PR objectives.- <div className="hidden xl:block overflow-hidden rounded-lg bg-white shadow overflow-x-auto"> - <Table className="min-w-full divide-y divide-gray-200"> + <div className="hidden xl:block rounded-lg bg-white shadow"> + <div className="overflow-x-auto"> + <Table className="w-full divide-y divide-gray-200">Also, add a closing
</div>after the Table closing tag.
🧹 Nitpick comments (1)
src/components/ValueSet/ValueSetList.tsx (1)
49-50: Improve badge hover states for accessibility.The hover states on badges may not be necessary since they aren't interactive elements.
- active: "bg-green-100 text-green-800 hover:bg-green-200", - draft: - "bg-yellow-100 text-yellow-800 hover:bg-yellow-200", - retired: "bg-red-100 text-red-800 hover:bg-red-200", - unknown: "bg-gray-100 text-gray-800 hover:bg-gray-200", + active: "bg-green-100 text-green-800", + draft: "bg-yellow-100 text-yellow-800", + retired: "bg-red-100 text-red-800", + unknown: "bg-gray-100 text-gray-800",Also applies to: 54-55
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Routers/AppRouter.tsx(1 hunks)src/components/Questionnaire/QuestionnaireList.tsx(3 hunks)src/components/ValueSet/ValueSetList.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Routers/AppRouter.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (11)
src/components/ValueSet/ValueSetList.tsx (7)
6-7: Import path updated correctly.The import path for CareIcon has been updated to use the absolute path with the @ prefix, which aligns with modern import practices.
19-24: Updated tooltip imports using absolute paths.The tooltip component imports have been properly updated to use absolute paths, which maintains consistency with the project's import conventions.
34-139: Well-implemented responsive card view for mobile screens.The
RenderCardcomponent effectively addresses the need for a mobile-friendly view of valuesets. The component:
- Properly displays only on mobile/small screens with
xl:hidden- Includes appropriate status badges with conditional styling
- Uses tooltips for long names to improve UX
- Includes relevant metadata in an organized layout
- Conditionally renders edit buttons for non-system valuesets
141-247: Thorough implementation of desktop table view.The
RenderTablecomponent properly handles the desktop view requirements, with:
- Appropriate display logic (
hidden xl:block)- Well-structured table headers and content
- Consistent use of tooltips for long content
- Empty state handling with a message when no valuesets are found
145-146: Table container doesn't create unnecessary scroll bars.The
RenderTablecomponent has correct overflow handling, fixing the reported issue by keeping the table container properly styled without creating extra scroll bars.
249-288: Component implementation follows best practices.The
ValueSetListfunction has been refactored to:
- Use the new rendering components
- Maintain the same data fetching logic
- Support both mobile and desktop views through responsive design
- Ensure loading states are handled appropriately
283-284: Good implementation of responsive rendering.The approach of using two different rendering components controlled by CSS display properties is an effective way to handle responsive layouts without JavaScript-based media queries.
src/components/Questionnaire/QuestionnaireList.tsx (4)
39-138: Card implementation has good mobile UX.The
RenderCardcomponent presents a clean, well-organized mobile view with:
- Appropriate status badges
- Clean information hierarchy
- Tooltips for truncated content
- Proper event handling to prevent event propagation
51-54: Card click behavior properly implemented.The card is correctly set as clickable with appropriate cursor styling and navigation handling.
140-217: Table implementation needs scroll fix but is otherwise well-structured.The
RenderTablecomponent provides a good desktop view with:
- Clear column headers
- Appropriate styling and hover states
- Tooltips for long content
- Empty state handling
However, it has the scrolling issue noted in the separate comment.
291-292: Responsive rendering pattern consistently applied.The pattern of using two separate rendering components for different screen sizes is consistently applied across both files, making maintenance easier.
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/Questionnaire/QuestionnaireList.tsx (1)
147-148:⚠️ Potential issueFix conflicting overflow styles causing extra scrollbar.
The table wrapper has conflicting overflow styles:
overflow-hiddenandoverflow-x-autoapplied to the same div, which is likely causing the extra scrollbar issue mentioned in the PR objectives.Apply this fix to resolve the scroll bar issue:
-<div className="hidden xl:block overflow-hidden rounded-lg bg-white shadow overflow-x-auto"> +<div className="hidden xl:block rounded-lg bg-white shadow"> + <div className="overflow-x-auto"> <Table className="min-w-full divide-y divide-gray-200"> {/* ... table content ... */} </Table> + </div> </div>
🧹 Nitpick comments (5)
src/components/Questionnaire/QuestionnaireList.tsx (5)
39-138: Good mobile-friendly card implementation, but should be moved to a separate file.The RenderCard component provides a well-structured mobile view for questionnaires. However, defining it within this file makes the QuestionnaireList component overly complex.
Consider extracting RenderCard to a separate file (e.g.,
QuestionnaireCard.tsx) to improve code organization and maintainability. This would also make it easier to test and reuse if needed.
119-121: Consider using a dedicated handler function for better readability.The inline event handler for stopping propagation and navigation is embedded directly in the onClick prop, making it harder to read.
Extract this logic to a named handler function at the component level:
const RenderCard = ({ questionnaireList, }: { questionnaireList: QuestionnaireDetail[]; }) => { const navigate = useNavigate(); + + const handleViewClick = (e: React.MouseEvent, slug: string) => { + e.stopPropagation(); + navigate(`/admin/questionnaire/${slug}/edit`); + }; return ( // ... <Button variant="outline" size="sm" - onClick={(e) => { - e.stopPropagation(); - navigate(`/admin/questionnaire/${questionnaire.slug}/edit`); - }} + onClick={(e) => handleViewClick(e, questionnaire.slug)} className="font-semibold shadow-gray-300 text-gray-950 border-gray-400" >
140-217: Consider applying React.memo to optimize rendering.The RenderTable component will re-render unnecessarily when parent component state changes unrelated to questionnaireList.
Optimize component rendering by applying React.memo and moving it to a separate file:
// In QuestionnaireTable.tsx import React from 'react'; // ... other imports const QuestionnaireTable = React.memo(({ questionnaireList, }: { questionnaireList: QuestionnaireDetail[]; }) => { // ... component logic }); export default QuestionnaireTable;Then import and use it in QuestionnaireList.tsx:
import QuestionnaireTable from './QuestionnaireTable'; // ... <QuestionnaireTable questionnaireList={questionnaireList} />
190-193: Inconsistent whitespace class usage in table cell.The div containing the description text uses both
whitespace-normalandtruncate, which are contradictory. Thetruncateclass enforces single-line text with ellipsis, whilewhitespace-normalallows text to wrap.Choose one approach for text handling:
-<div className="truncate text-sm text-gray-900 break-words whitespace-normal"> +<div className="text-sm text-gray-900 break-words whitespace-normal">Or if you want truncated text:
-<div className="truncate text-sm text-gray-900 break-words whitespace-normal"> +<div className="truncate text-sm text-gray-900">
192-193: Ensure consistent line clamp behavior between mobile and desktop views.The mobile card view uses
line-clamp-2for description (line 109), but the table view doesn't limit text lines, leading to inconsistent user experience across devices.Apply consistent text truncation to both views:
-<div className="truncate text-sm text-gray-900 break-words whitespace-normal"> +<div className="text-sm text-gray-900 line-clamp-2"> {questionnaire.description} </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Questionnaire/QuestionnaireList.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
- GitHub Check: lint
- GitHub Check: OSSAR-Scan
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
src/components/Questionnaire/QuestionnaireList.tsx (1)
291-292: LGTM! Good implementation of responsive design.The component now properly handles different screen sizes by conditionally rendering either a table or card layout. This is a significant improvement for mobile users.
|
LGTM |
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
|
@Rishith25 fix the merge conflict |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Questionnaire/QuestionnaireList.tsx (1)
194-201:⚠️ Potential issueAdd stopPropagation to View button in table row.
The View button needs a stopPropagation handler to prevent triggering both the button action and the row's onClick navigation when clicked.
<Button variant="outline" size="sm" + onClick={(e) => { + e.stopPropagation(); + navigate(`/admin/questionnaire/${questionnaire.slug}/edit`); + }} className="font-semibold shadow-gray-300 text-gray-950 border-gray-400" > <EyeIcon className="w-4 h-4 mr-0" /> {t("View")} </Button>
🧹 Nitpick comments (3)
src/components/ValueSet/ValueSetList.tsx (2)
50-53: Make card clickable for better user experience.The card is styled with hover effects suggesting it's interactive, but lacks an onClick handler to navigate to the edit page. This creates a disconnected experience where only the edit button is clickable.
- <Card - key={valueset.id} - className="overflow-hidden bg-white rounded-lg transition-shadow hover:shadow-lg" - > + <Card + key={valueset.id} + className="overflow-hidden bg-white rounded-lg cursor-pointer transition-shadow hover:shadow-lg" + onClick={() => navigate(`/admin/valuesets/${valueset.slug}/edit`)} + >
147-256: Fix table overflow and styling issues.The table wrapper has conflicting overflow classes that may cause issues with horizontal scrolling on some screen sizes.
- <div className="hidden xl:block overflow-hidden rounded-lg bg-white shadow"> + <div className="hidden xl:block rounded-lg bg-white shadow"> + <div className="overflow-x-auto"> <Table className="min-w-full divide-y divide-gray-200"> {/* ...table content... */} </Table> + </div> </div>src/components/Questionnaire/QuestionnaireList.tsx (1)
147-148: Fix table overflow issues.The table wrapper has conflicting overflow classes (
overflow-hiddenandoverflow-x-auto) which can cause issues with horizontal scrolling.- <div className="hidden xl:block overflow-hidden rounded-lg bg-white shadow overflow-x-auto"> - <Table className="min-w-full divide-y divide-gray-200"> + <div className="hidden xl:block rounded-lg bg-white shadow"> + <div className="overflow-x-auto"> + <Table className="w-full divide-y divide-gray-200"></TableBody> </Table> + </div> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Questionnaire/QuestionnaireList.tsx(3 hunks)src/components/ValueSet/ValueSetList.tsx(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (10)
src/components/ValueSet/ValueSetList.tsx (6)
46-146: Table and card responsive pattern is well-implemented.The implementation of a card-based layout for mobile views using the
xl:hiddenutility class provides good responsive behavior. The structured information display with sections for name, slug, system definition, and description maintains content hierarchy even on smaller screens.
58-68: Remove hover effects from non-interactive badge elements.The status badges include hover effects (
hover:bg-*-200) but are not interactive elements. This can create a misleading user experience where users expect an action when hovering over the badge.- { - active: "bg-green-100 text-green-800 hover:bg-green-200", - draft: - "bg-yellow-100 text-yellow-800 hover:bg-yellow-200", - retired: "bg-red-100 text-red-800 hover:bg-red-200", - unknown: "bg-gray-100 text-gray-800 hover:bg-gray-200", - }[valueset.status] + { + active: "bg-green-100 text-green-800", + draft: "bg-yellow-100 text-yellow-800", + retired: "bg-red-100 text-red-800", + unknown: "bg-gray-100 text-gray-800", + }[valueset.status]
207-219: Remove hover effects from non-interactive badge elements in the table view.Similar to the card view, the status badges in the table include hover effects but are not interactive elements, creating a misleading user experience.
- { - active: - "bg-green-100 text-green-800 hover:bg-green-200", - draft: - "bg-yellow-100 text-yellow-800 hover:bg-yellow-200", - retired: "bg-red-100 text-red-800 hover:bg-red-200", - unknown: "bg-gray-100 text-gray-800 hover:bg-gray-200", - }[valueset.status] + { + active: "bg-green-100 text-green-800", + draft: "bg-yellow-100 text-yellow-800", + retired: "bg-red-100 text-red-800", + unknown: "bg-gray-100 text-gray-800", + }[valueset.status]
231-240: Add stopPropagation to edit button click handler.If you implement clickable card rows, the edit button will need stopPropagation to prevent triggering both actions. This ensures that only the button's action is triggered, not the card's.
<Button variant="outline" size="sm" - onClick={() => + onClick={(e) => { + e.stopPropagation(); navigate(`/admin/valuesets/${valueset.slug}/edit`) - } + }} > <Pencil className="w-4 h-4 mr-0" /> {t("edit")} </Button>
338-339: Responsive rendering implementation is good.The implementation of both table and card views with appropriate responsive utility classes ensures good UI/UX across different device sizes. The conditional rendering based on screen size is a good approach.
285-286: Good responsive layout for filtering and actions.The update to use
flex-col md:flex-rowfor the filter controls provides a better experience on mobile devices while maintaining a clean layout on larger screens.src/components/Questionnaire/QuestionnaireList.tsx (4)
39-138: Card implementation for mobile view looks good, but has clickable nested elements.The card implementation for mobile view provides a good responsive solution. However, the card has both an onClick handler and a child button with its own onClick handler, which can lead to confusing interactions. The e.stopPropagation() on the button is correctly implemented to prevent this issue.
58-69: Remove hover effects from non-interactive badge elements.The status badges include hover effects (
hover:bg-*-200) but are not interactive elements. This creates a misleading user experience where users expect an action when hovering over the badge.- { - active: "bg-green-100 text-green-800 hover:bg-green-200", - draft: - "bg-yellow-100 text-yellow-800 hover:bg-yellow-200", - retired: "bg-red-100 text-red-800 hover:bg-red-200", - }[questionnaire.status] + { + active: "bg-green-100 text-green-800", + draft: "bg-yellow-100 text-yellow-800", + retired: "bg-red-100 text-red-800", + }[questionnaire.status]
139-217: Functions outside of main component is good for memoization.Moving the
RenderCardandRenderTablefunctions outside the main component allows React to properly memoize them, which is good for performance. This addresses a previous review comment about not defining functions inside components.
292-293: Responsive rendering implementation is well done.The implementation of both table and card views with appropriate responsive utility classes ensures good UI/UX across different device sizes. This approach provides a better user experience on both mobile and desktop.
@khavinshankar Resolved the merge conflicts |
|
@Rishith25 Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Screenshots
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit