Enhanced Styling Updates: Conditional 'Completed' Tags and Export Button Design #9499
Conversation
…esource and Shifting sections
WalkthroughThe pull request introduces visual enhancements across multiple components, focusing on improving the representation of completed states and export button interactions. Changes span four components: Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/components/Shifting/ShiftingBoard.tsx (1)
Line range hint
176-182: Enhance drag-and-drop user experienceThe current implementation could benefit from better error handling and user feedback.
Consider these improvements:
onDragEnd={(result) => { - if (result.source.droppableId !== result.destination?.droppableId) + if (result.source.droppableId !== result.destination?.droppableId) { + const updateStatus = async () => { + try { + // Show loading state + setIsUpdating(true); navigate( `/shifting/${result.draggableId}/update?status=${result.destination?.droppableId}`, ); + } catch (error) { + // Show error notification + showErrorNotification(t('status_update_failed')); + } finally { + setIsUpdating(false); + } + }; + updateStatus(); + } }}Also consider adding visual feedback during drag operations using the
isDraggingprop from the drag context.
🧹 Nitpick comments (5)
src/components/Common/Export.tsx (1)
133-137: Simplify conditional class names using template literalsThe current nested ternary expression could be simplified for better readability.
- className={`tooltip mx-2 p-4 text-lg ${ - isCompleted - ? "text-white" - : "text-secondary-800 disabled:bg-transparent disabled:text-secondary-500" - }`} + className={`tooltip mx-2 p-4 text-lg ${ + isCompleted ? "text-white" : "text-secondary-800" + } disabled:bg-transparent disabled:text-secondary-500`}src/components/Resource/ResourceBoard.tsx (1)
Line range hint
119-131: Consider caching the export action callbackThe export action callback is recreated on every render. Consider memoizing it for better performance.
+ const getExportAction = useCallback((board: string) => async () => { + const { data } = await request(routes.downloadResourceRequests, { + query: { + ...formatFilter({ ...qParams, status: board }), + csv: true, + }, + }); + return data ?? null; + }, [qParams]); - action={async () => { - const { data } = await request(routes.downloadResourceRequests, { - query: { - ...formatFilter({ ...qParams, status: board }), - csv: true, - }, - }); - return data ?? null; - }} + action={getExportAction(board)}src/components/Shifting/ShiftingBoard.tsx (3)
150-150: Enhance tooltip accessibility and internationalizationThe tooltip text should be translated and more descriptive to improve user experience.
Consider applying this change:
- tooltip={`${board.text}`} + tooltip={t('export_shifts_for_status', { status: t(board.text.toLowerCase()) })}
Line range hint
54-69: Consider extracting status configurationThe wartime/peacetime status configuration could be more maintainable and type-safe.
Consider creating a dedicated configuration module:
// src/config/shiftingStatus.ts export interface ShiftingStatusConfig { completed: string[]; active: string[]; isCompleted: (status: string) => boolean; } export const getShiftingStatusConfig = (isWartime: boolean): ShiftingStatusConfig => ({ completed: isWartime ? ["COMPLETED", "REJECTED", "CANCELLED", "DESTINATION REJECTED", "PATIENT EXPIRED"] : ["CANCELLED", "PATIENT EXPIRED"], active: [...], // Define active statuses isCompleted: (status: string) => completed.includes(status) });
Line range hint
36-201: Consider component decomposition and performance optimizationThe BoardView component handles multiple responsibilities and could benefit from decomposition and optimization.
Consider these improvements:
- Extract the status management logic into a custom hook:
// useShiftingStatus.ts export const useShiftingStatus = (isWartime: boolean) => { const [activeFilter, setActiveFilter] = useState<boolean>(true); const statusConfig = useMemo(() => getShiftingStatusConfig(isWartime), [isWartime]); const currentBoards = useMemo(() => activeFilter ? statusConfig.active : statusConfig.completed, [activeFilter, statusConfig] ); return { currentBoards, setActiveFilter, statusConfig }; };
- Extract the header section into a separate component:
const ShiftingBoardHeader: React.FC<{ onViewChange: () => void; onSearch: (value: string) => void; onFilterChange: () => void; }> = memo(({ onViewChange, onSearch, onFilterChange }) => { // Header rendering logic });
- Memoize callbacks and derived state:
+ const handleSearch = useCallback((e) => + updateQuery({ [e.name]: e.value }), + [updateQuery] + ); + const exportAction = useCallback(async () => { + const { data } = await request(routes.downloadShiftRequests, { + query: { ...formatFilter(qParams), csv: true }, + }); + return data ?? null; + }, [qParams]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Common/Export.tsx(2 hunks)src/components/Kanban/Board.tsx(1 hunks)src/components/Resource/ResourceBoard.tsx(1 hunks)src/components/Shifting/ShiftingBoard.tsx(1 hunks)
🔇 Additional comments (1)
src/components/Shifting/ShiftingBoard.tsx (1)
Line range hint 11-11: Request for additional context
The changes to ExportButton component in Export.tsx were mentioned in the AI summary but the file is not provided for review. Please provide the Export.tsx file to ensure the tooltip implementation is consistent with the changes made there.
✅ Verification successful
No changes needed in other files
The Export.tsx file shows that the ExportButton component already has proper tooltip implementation with the tooltipClassName prop and tooltip text display. The changes in ShiftingBoard.tsx are consistent with the existing implementation in Export.tsx. No other files need modifications as they are already using the same component interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for recent changes to Export.tsx
git log -p -- src/components/Common/Export.tsx
Length of output: 0
Script:
#!/bin/bash
# Search for Export.tsx files in the repository
fd -e tsx -e ts "Export"
# Search for ExportButton component usage
rg -l "ExportButton|Export\.tsx" --type ts --type tsx
Length of output: 165
Script:
#!/bin/bash
# Check the content of Export.tsx
cat src/components/Common/Export.tsx
# Search for ExportButton usage with correct file type
rg -t ts "ExportButton"
Length of output: 5742
| @@ -113,7 +113,7 @@ export const ExportButton = ({ | |||
| ...props | |||
| }: ExportButtonProps) => { | |||
| const { isExporting, exportFile } = useExport(); | |||
|
|
|||
| const isCompleted = props.tooltip?.toLowerCase() === "completed"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a dedicated prop for completion status
Using the tooltip text to determine completion status mixes presentation with business logic. This approach is fragile and could break if tooltip text changes.
Consider adding a dedicated prop:
- const isCompleted = props.tooltip?.toLowerCase() === "completed";
+ interface ExportButtonProps {
+ // ... existing props
+ isCompleted?: boolean;
+ }
+
+ export const ExportButton = ({
+ isCompleted = false,
+ // ... other props
+ }: ExportButtonProps)Committable suggestion skipped: line range outside the PR's diff.
| <div | ||
| className={`mx-2 flex items-center justify-between rounded-lg border p-4 ${ | ||
| section.id === "COMPLETED" | ||
| ? "border-green-500 bg-green-500 " | ||
| : "border-secondary-300 bg-white" | ||
| }`} | ||
| > | ||
| <div> | ||
| {section.id === "COMPLETED" ? ( | ||
| <span className="font-bold text-white">{section.title}</span> | ||
| ) : ( | ||
| section.title | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract status constants and simplify conditional rendering
The current implementation has several areas for improvement:
- Hardcoded "COMPLETED" string
- Complex nested conditional rendering
- Multiple nested divs
Consider this refactor:
+ const SECTION_STATUS = {
+ COMPLETED: 'COMPLETED'
+ } as const;
+
+ const getSectionHeaderClass = (status: string) =>
+ status === SECTION_STATUS.COMPLETED
+ ? "border-green-500 bg-green-500"
+ : "border-secondary-300 bg-white";
- <div
- className={`mx-2 flex items-center justify-between rounded-lg border p-4 ${
- section.id === "COMPLETED"
- ? "border-green-500 bg-green-500 "
- : "border-secondary-300 bg-white"
- }`}
- >
- <div>
- {section.id === "COMPLETED" ? (
- <span className="font-bold text-white">{section.title}</span>
- ) : (
- section.title
- )}
- </div>
+ <div className={`mx-2 flex items-center justify-between rounded-lg border p-4 ${getSectionHeaderClass(section.id)}`}>
+ <span className={section.id === SECTION_STATUS.COMPLETED ? "font-bold text-white" : ""}>
+ {section.title}
+ </span>📝 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.
| <div | |
| className={`mx-2 flex items-center justify-between rounded-lg border p-4 ${ | |
| section.id === "COMPLETED" | |
| ? "border-green-500 bg-green-500 " | |
| : "border-secondary-300 bg-white" | |
| }`} | |
| > | |
| <div> | |
| {section.id === "COMPLETED" ? ( | |
| <span className="font-bold text-white">{section.title}</span> | |
| ) : ( | |
| section.title | |
| )} | |
| </div> | |
| const SECTION_STATUS = { | |
| COMPLETED: 'COMPLETED' | |
| } as const; | |
| const getSectionHeaderClass = (status: string) => | |
| status === SECTION_STATUS.COMPLETED | |
| ? "border-green-500 bg-green-500" | |
| : "border-secondary-300 bg-white"; | |
| <div className={`mx-2 flex items-center justify-between rounded-lg border p-4 ${getSectionHeaderClass(section.id)}`}> | |
| <span className={section.id === SECTION_STATUS.COMPLETED ? "font-bold text-white" : ""}> | |
| {section.title} | |
| </span> |
| @@ -116,6 +116,7 @@ export default function BoardView() { | |||
| <h3 className="flex h-8 items-center text-xs"> | |||
| {board}{" "} | |||
| <ExportButton | |||
| tooltip={`${board}`} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add translation and improve accessibility for tooltips
The tooltip text should be translated and accessible.
- tooltip={`${board}`}
+ tooltip={t(`resource_board_export_tooltip`, { board: t(board.toLowerCase()) })}
+ aria-label={t(`resource_board_export_aria_label`, { board: t(board.toLowerCase()) })}Also, ensure these translation keys are added to your i18n files:
{
"resource_board_export_tooltip": "Export {{board}} resources",
"resource_board_export_aria_label": "Export resources from {{board}} board"
}
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Updated Design
Merge Checklist
Summary by CodeRabbit
New Features
ExportButtonwith a tooltip displaying the current board's name.Bug Fixes