fix: address code review feedback
- Replace type assertions with type guards in types.ts (isDateString, isStringArray) - Add useCallback for event handlers (handleTaskClick, handleKeyDown) - Replace styled-jsx with CSS modules (gantt.module.css) - Update tests to use CSS module class name patterns
This commit is contained in:
@@ -91,7 +91,7 @@ describe("GanttChart", () => {
|
|||||||
render(<GanttChart tasks={tasks} />);
|
render(<GanttChart tasks={tasks} />);
|
||||||
|
|
||||||
const taskRow = screen.getAllByText("Completed Task")[0].closest("[role='row']");
|
const taskRow = screen.getAllByText("Completed Task")[0].closest("[role='row']");
|
||||||
expect(taskRow).toHaveClass(/completed/i);
|
expect(taskRow?.className).toMatch(/Completed/i);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("should visually distinguish in-progress tasks", () => {
|
it("should visually distinguish in-progress tasks", () => {
|
||||||
@@ -106,7 +106,7 @@ describe("GanttChart", () => {
|
|||||||
render(<GanttChart tasks={tasks} />);
|
render(<GanttChart tasks={tasks} />);
|
||||||
|
|
||||||
const taskRow = screen.getAllByText("Active Task")[0].closest("[role='row']");
|
const taskRow = screen.getAllByText("Active Task")[0].closest("[role='row']");
|
||||||
expect(taskRow).toHaveClass(/in-progress/i);
|
expect(taskRow?.className).toMatch(/InProgress/i);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -3,7 +3,8 @@
|
|||||||
import type { GanttTask, GanttChartProps, TimelineRange, GanttBarPosition } from "./types";
|
import type { GanttTask, GanttChartProps, TimelineRange, GanttBarPosition } from "./types";
|
||||||
import { TaskStatus } from "@mosaic/shared";
|
import { TaskStatus } from "@mosaic/shared";
|
||||||
import { formatDate, isPastTarget, isApproachingTarget } from "@/lib/utils/date-format";
|
import { formatDate, isPastTarget, isApproachingTarget } from "@/lib/utils/date-format";
|
||||||
import { useMemo } from "react";
|
import { useMemo, useCallback } from "react";
|
||||||
|
import styles from "./gantt.module.css";
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Represents a dependency line between two tasks
|
* Represents a dependency line between two tasks
|
||||||
@@ -111,11 +112,11 @@ function getStatusClass(status: TaskStatus): string {
|
|||||||
function getRowStatusClass(status: TaskStatus): string {
|
function getRowStatusClass(status: TaskStatus): string {
|
||||||
switch (status) {
|
switch (status) {
|
||||||
case TaskStatus.COMPLETED:
|
case TaskStatus.COMPLETED:
|
||||||
return "gantt-row-completed";
|
return styles.rowCompleted;
|
||||||
case TaskStatus.IN_PROGRESS:
|
case TaskStatus.IN_PROGRESS:
|
||||||
return "gantt-row-in-progress";
|
return styles.rowInProgress;
|
||||||
case TaskStatus.PAUSED:
|
case TaskStatus.PAUSED:
|
||||||
return "gantt-row-paused";
|
return styles.rowPaused;
|
||||||
default:
|
default:
|
||||||
return "";
|
return "";
|
||||||
}
|
}
|
||||||
@@ -232,20 +233,26 @@ export function GanttChart({
|
|||||||
[showDependencies, sortedTasks, timelineRange]
|
[showDependencies, sortedTasks, timelineRange]
|
||||||
);
|
);
|
||||||
|
|
||||||
const handleTaskClick = (task: GanttTask) => (): void => {
|
const handleTaskClick = useCallback(
|
||||||
if (onTaskClick) {
|
(task: GanttTask) => (): void => {
|
||||||
onTaskClick(task);
|
|
||||||
}
|
|
||||||
};
|
|
||||||
|
|
||||||
const handleKeyDown = (task: GanttTask) => (e: React.KeyboardEvent<HTMLDivElement>): void => {
|
|
||||||
if (e.key === "Enter" || e.key === " ") {
|
|
||||||
e.preventDefault();
|
|
||||||
if (onTaskClick) {
|
if (onTaskClick) {
|
||||||
onTaskClick(task);
|
onTaskClick(task);
|
||||||
}
|
}
|
||||||
}
|
},
|
||||||
};
|
[onTaskClick]
|
||||||
|
);
|
||||||
|
|
||||||
|
const handleKeyDown = useCallback(
|
||||||
|
(task: GanttTask) => (e: React.KeyboardEvent<HTMLDivElement>): void => {
|
||||||
|
if (e.key === "Enter" || e.key === " ") {
|
||||||
|
e.preventDefault();
|
||||||
|
if (onTaskClick) {
|
||||||
|
onTaskClick(task);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
},
|
||||||
|
[onTaskClick]
|
||||||
|
);
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<div
|
<div
|
||||||
@@ -415,19 +422,6 @@ export function GanttChart({
|
|||||||
</div>
|
</div>
|
||||||
</div>
|
</div>
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
{/* CSS for status classes */}
|
|
||||||
<style jsx>{`
|
|
||||||
.gantt-row-completed {
|
|
||||||
background-color: #f0fdf4;
|
|
||||||
}
|
|
||||||
.gantt-row-in-progress {
|
|
||||||
background-color: #eff6ff;
|
|
||||||
}
|
|
||||||
.gantt-row-paused {
|
|
||||||
background-color: #fefce8;
|
|
||||||
}
|
|
||||||
`}</style>
|
|
||||||
</div>
|
</div>
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|||||||
12
apps/web/src/components/gantt/gantt.module.css
Normal file
12
apps/web/src/components/gantt/gantt.module.css
Normal file
@@ -0,0 +1,12 @@
|
|||||||
|
/* Gantt Chart Status Row Styles */
|
||||||
|
.rowCompleted {
|
||||||
|
background-color: #f0fdf4; /* green-50 */
|
||||||
|
}
|
||||||
|
|
||||||
|
.rowInProgress {
|
||||||
|
background-color: #eff6ff; /* blue-50 */
|
||||||
|
}
|
||||||
|
|
||||||
|
.rowPaused {
|
||||||
|
background-color: #fefce8; /* yellow-50 */
|
||||||
|
}
|
||||||
@@ -58,31 +58,49 @@ export interface GanttChartProps {
|
|||||||
showDependencies?: boolean;
|
showDependencies?: boolean;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Type guard to check if a value is a valid date string
|
||||||
|
*/
|
||||||
|
function isDateString(value: unknown): value is string {
|
||||||
|
return typeof value === 'string' && !isNaN(Date.parse(value));
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Type guard to check if a value is an array of strings
|
||||||
|
*/
|
||||||
|
function isStringArray(value: unknown): value is string[] {
|
||||||
|
return Array.isArray(value) && value.every((item) => typeof item === 'string');
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Helper to convert a base Task to GanttTask
|
* Helper to convert a base Task to GanttTask
|
||||||
* Uses createdAt as startDate if not in metadata, dueDate as endDate
|
* Uses createdAt as startDate if not in metadata, dueDate as endDate
|
||||||
*/
|
*/
|
||||||
export function toGanttTask(task: Task): GanttTask | null {
|
export function toGanttTask(task: Task): GanttTask | null {
|
||||||
// For Gantt chart, we need both start and end dates
|
// For Gantt chart, we need both start and end dates
|
||||||
const startDate =
|
const metadataStartDate = task.metadata?.startDate;
|
||||||
(task.metadata?.startDate as string | undefined)
|
const startDate = isDateString(metadataStartDate)
|
||||||
? new Date(task.metadata.startDate as string)
|
? new Date(metadataStartDate)
|
||||||
: task.createdAt;
|
: task.createdAt;
|
||||||
|
|
||||||
const endDate = task.dueDate || new Date();
|
const endDate = task.dueDate ?? new Date();
|
||||||
|
|
||||||
// Validate dates
|
// Validate dates
|
||||||
if (!startDate || !endDate) {
|
if (!startDate || !endDate) {
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Extract dependencies with type guard
|
||||||
|
const metadataDependencies = task.metadata?.dependencies;
|
||||||
|
const dependencies = isStringArray(metadataDependencies)
|
||||||
|
? metadataDependencies
|
||||||
|
: undefined;
|
||||||
|
|
||||||
return {
|
return {
|
||||||
...task,
|
...task,
|
||||||
startDate,
|
startDate,
|
||||||
endDate,
|
endDate,
|
||||||
dependencies: Array.isArray(task.metadata?.dependencies)
|
dependencies,
|
||||||
? (task.metadata.dependencies as string[])
|
|
||||||
: undefined,
|
|
||||||
isMilestone: task.metadata?.isMilestone === true,
|
isMilestone: task.metadata?.isMilestone === true,
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user