feat: add dashboard layout with calendar, notes, and improved sidebar#115
feat: add dashboard layout with calendar, notes, and improved sidebar#115
Conversation
- Extract QuickActionsDialog to separate component - Add AppHeader with Quick actions button, Add button, and Export button - Lift quickActionsOpen state to RootPage for proper overlay positioning - Remove QuickActionsDialog from AppSidebar Co-Authored-By: martty-code <nesalia.inc@gmail.com> Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
- Add two-column 80/20 layout to home page with calendar and notes - Create NotesList component with fullscreen dialog and checkbox completion - Add tooltips to header buttons (create new, export) - Add active state highlighting to sidebar navigation items Co-Authored-By: martty-code <nesalia.inc@gmail.com> Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
![marty-action[bot]](https://avatars.githubusercontent.com/in/2883066?s=60&v=4){kind=small}
| const navigate = useNavigate(); | ||
| const openDialog = useDialogStore((state) => state.openDialog); | ||
|
|
||
| const quickActions: QuickAction[] = [ |
There was a problem hiding this comment.
The quickActions array is missing TypeScript type safety. Items with heading property don't match the QuickAction interface.
Consider either:
- Creating a union type that includes heading items
- Using a type assertion
type QuickActionItem = QuickAction | { heading: string };
const quickActions: QuickActionItem[] = [...]
| }, | ||
| ]; | ||
|
|
||
| const groupedActions = quickActions.reduce<{ heading?: string; items: QuickAction[] }[]>( |
There was a problem hiding this comment.
The reduce operation could be simplified and made more type-safe by defining proper types for the accumulator.
interface ActionGroup {
heading?: string;
items: QuickAction[];
}This would make the code more maintainable and self-documenting.
|
|
||
| export function NotesList({ notes }: NotesListProps) { | ||
| const [selectedNote, setSelectedNote] = useState<Note | null>(null); | ||
| const [completedNotes, setCompletedNotes] = useState<Set<string>>(new Set()); |
There was a problem hiding this comment.
The completedNotes state is local to this component and will be lost when the component unmounts. Consider persisting this state (e.g., to localStorage) or passing the completion state from the parent component if persistence is desired.
| <CardTitle>Notes</CardTitle> | ||
| </CardHeader> | ||
| <CardContent className="border-t px-0"> | ||
| <NotesList |
There was a problem hiding this comment.
Hardcoded notes data should be moved to a centralized data source or retrieved from an API/database. This would allow for:
- Dynamic notes management
- Persistence across sessions
- Better separation of concerns
Consider creating a notes hook (useNotes) similar to the existing useAlerts, useEmployees hooks.
| } | ||
|
|
||
| export const AppHeader = ({ onQuickActionsClick }: AppHeaderProps) => { | ||
| React.useEffect(() => { |
There was a problem hiding this comment.
The keyboard shortcut for Cmd+K / Ctrl+K is good UX. However, consider adding this to a quick actions documentation or tooltip hint so users discover this feature.
Also, the effect dependency array includes onQuickActionsClick which may cause unnecessary effect re-runs if the parent re-renders. Consider using useCallback in the parent or wrapping in useRef.
| /> | ||
| )} | ||
| <Sidebar | ||
| className="top-(--header-height) h-[calc(100svh-var(--header-height))]!" |
There was a problem hiding this comment.
There's an unusual Tailwind class syntax here: top-(--header-height) h-[calc(100svh-var(--header-height))]!
The exclamation mark at the end and parentheses around the CSS variable seem incorrect. This should likely be:
h-[calc(100svh-var(--header-height))]
Please verify this renders correctly in production.
| sidebarStore.getServerSnapshot, | ||
| ); | ||
|
|
||
| const [quickActionsOpen, setQuickActionsOpen] = useState(false); |
There was a problem hiding this comment.
Good use of useSyncExternalStore for sidebar state persistence! This is a proper pattern that avoids hydration mismatches and provides synchronous access to localStorage state.
Consider extracting this custom store pattern to a reusable hook in src/renderer/src/hooks/ for better maintainability (e.g., useLocalStorageStore).
| }; | ||
|
|
||
| // Get counts for sidebar badges | ||
| const { data: alerts = [] } = useAlerts(); |
There was a problem hiding this comment.
The isActive function for determining active navigation state is simple and effective. However, it may cause issues with routes like /employees and /employees/:id both being highlighted.
Consider using the router's match functionality or a more precise path matching algorithm if you need to distinguish between list and detail views.
SummaryThis PR adds a dashboard layout with a two-column 80/20 split, featuring a calendar and notes panel on the right side, along with improved header and sidebar components. The code is generally well-structured and follows the project's existing patterns. Critical IssuesNone - This PR can be merged as-is. The following are suggestions for future improvements. RecommendationsCode Quality & Type Safety
Performance
Positive Notes
Test CoverageNo new tests were added for these UI changes. Consider adding tests for:
|
- Add footer with copyright, Help/Privacy/Terms links, online status, and version - Move write mode indicator from sidebar to footer - Remove TanStack Router devtools from codebase Co-Authored-By: martty-code <nesalia.inc@gmail.com> Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
Add no-drag CSS class and apply proper drag regions to ensure window can be dragged while keeping buttons clickable Co-Authored-By: martty-code <nesalia.inc@gmail.com> Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
| import { AppSidebar } from "@/components/app-sidebar"; | ||
| import { AppHeader } from "@/components/app-header"; | ||
| import { DialogManager } from "@/components/dialogs/DialogManager"; | ||
| import { RightSidebar } from "@/components/right-sidebar"; |
There was a problem hiding this comment.
The RightSidebar component is imported here but the file doesn't exist in the repository. This will cause a build/runtime error. Either create the missing component or remove this import and the conditional rendering at line 167.
| import { AppSidebar } from "@/components/app-sidebar"; | ||
| import { AppHeader } from "@/components/app-header"; | ||
| import { DialogManager } from "@/components/dialogs/DialogManager"; | ||
| import { RightSidebar } from "@/components/right-sidebar"; |
There was a problem hiding this comment.
The RightSidebar component is imported here but the file doesn't exist in the repository. This will cause a build/runtime error. Either create the missing component or remove this import and the conditional rendering at line 167.
| notes={[ | ||
| { | ||
| id: "1", | ||
| title: "Réunion équipe", |
There was a problem hiding this comment.
The notes data contains hardcoded French text while the rest of the app uses i18n. Either use translation keys or move this data to a proper i18n structure for consistency.
| const navigate = useNavigate(); | ||
| const openDialog = useDialogStore((state) => state.openDialog); | ||
|
|
||
| const quickActions: QuickAction[] = [ |
There was a problem hiding this comment.
The quickActions array is recreated on every render. Wrap it in useMemo for better performance: const quickActions = React.useMemo(() => [...], []);
| import { Command, CommandEmpty, CommandGroup, CommandItem, CommandList } from "cmdk"; | ||
| import { useDialogStore } from "@/stores/dialog-store"; | ||
|
|
||
| interface QuickAction { |
There was a problem hiding this comment.
The QuickAction interface allows heading to be mixed with regular actions. Consider using a discriminated union for better type safety: type QuickAction = { heading: string } | { id: string; title: string; ... };
| <div className="h-6 w-px bg-border" /> | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <Button variant="outline" className="gap-2"> |
There was a problem hiding this comment.
The Export button has no onClick handler - it's a non-functional UI element. Either add the export functionality or disable the button with disabled attribute until implemented.
| <AppSidebar /> | ||
| <SidebarInset className="flex flex-1 flex-col"> | ||
| {/*<header className="h-14 border-b"></header>*/} | ||
| <AppHeader |
There was a problem hiding this comment.
The AppHeader component is called with onToggleRightSidebar and rightSidebarOpen props that are not defined in its interface. Update AppHeaderProps to include these props.
| <div className="flex items-center gap-3"> | ||
| <span>© 2024 WEMS</span> | ||
| <span className="text-border">|</span> | ||
| <button className="hover:text-foreground transition-colors"> |
There was a problem hiding this comment.
These footer buttons have no click handlers. Either add functionality or disable them until implemented.
| export const AppHeader = ({ onQuickActionsClick }: AppHeaderProps) => { | ||
| React.useEffect(() => { | ||
| // Handle Cmd+K / Ctrl+K keyboard shortcut | ||
| const handleKeyDown = (event: KeyboardEvent) => { |
There was a problem hiding this comment.
Consider using useCallback for handleKeyDown to ensure stable reference: const handleKeyDown = React.useCallback((event: KeyboardEvent) => { ... }, [onQuickActionsClick]);
SummaryThis PR adds several UI improvements to the dashboard including a new header component, calendar and notes widgets in an 80/20 layout, active state highlighting for sidebar navigation, and a refactored quick actions dialog. The code is generally well-structured and follows existing patterns. Critical Issues1. Missing RightSidebar Component File📁 The code imports Recommendation: Either create the missing 2. AppHeader Props Mismatch📁 The Recommendation: Update the 3. Hardcoded French Content📁 The notes data contains hardcoded French text while the rest of the app uses i18n. Recommendation: Use translation keys or move this data to a proper i18n structure. RecommendationsPerformance
Type Safety
Functionality
Positive Notes✅ Good component extraction - Security✅ No security issues identified - proper input handling, no XSS vulnerabilities, no hardcoded secrets Testing
Overall Assessment: The PR introduces good UX improvements but has some critical issues (missing file, prop mismatches) that need to be addressed before merging. The code quality is generally good with room for some performance optimizations. |
1 participant
Summary
Test plan
🤖 Generated with Claude Code