From fc9301fda2d3656254f9078db4ecb426e0f9c461 Mon Sep 17 00:00:00 2001 From: Loris Van Katwijk Date: Mon, 6 May 2024 16:39:27 +0200 Subject: [PATCH] fix: Resolve issues highlighted in PR review --- src/app/(dashboard)/jobmonitor/error.tsx | 22 -------- src/app/(dashboard)/jobmonitor/layout.tsx | 7 --- src/app/(dashboard)/jobmonitor/page.tsx | 5 -- src/app/(dashboard)/layout.tsx | 2 +- src/app/(dashboard)/page.tsx | 14 ++++-- .../applications/ApplicationList.ts | 3 +- src/components/layout/OIDCSecure.tsx | 1 + src/components/ui/ApplicationDialog.tsx | 30 +++++++---- src/components/ui/DashboardDrawer.tsx | 50 +++++++++++++------ src/components/ui/DiracLogo.tsx | 16 ------ src/components/ui/DrawerItem.tsx | 29 ++++++++++- src/components/ui/DrawerItemGroup.tsx | 33 ++++++------ src/contexts/ApplicationsProvider.tsx | 4 +- src/hooks/theme.tsx | 13 +++-- src/types/ApplicationConfig.ts | 8 +++ test/unit-tests/Dashboard.test.tsx | 40 ++++++++------- test/unit-tests/DiracLogo.test.tsx | 22 -------- 17 files changed, 152 insertions(+), 147 deletions(-) delete mode 100644 src/app/(dashboard)/jobmonitor/error.tsx delete mode 100644 src/app/(dashboard)/jobmonitor/layout.tsx delete mode 100644 src/app/(dashboard)/jobmonitor/page.tsx delete mode 100644 src/components/ui/DiracLogo.tsx create mode 100644 src/types/ApplicationConfig.ts delete mode 100644 test/unit-tests/DiracLogo.test.tsx diff --git a/src/app/(dashboard)/jobmonitor/error.tsx b/src/app/(dashboard)/jobmonitor/error.tsx deleted file mode 100644 index 68cea11a..00000000 --- a/src/app/(dashboard)/jobmonitor/error.tsx +++ /dev/null @@ -1,22 +0,0 @@ -"use client"; - -import { useEffect } from "react"; - -export default function Error({ - error, - reset, -}: { - error: Error; - reset: () => void; -}) { - useEffect(() => { - console.error(error); - }, [error]); - - return ( -
-

Something went wrong!

- -
- ); -} diff --git a/src/app/(dashboard)/jobmonitor/layout.tsx b/src/app/(dashboard)/jobmonitor/layout.tsx deleted file mode 100644 index 1f26a53c..00000000 --- a/src/app/(dashboard)/jobmonitor/layout.tsx +++ /dev/null @@ -1,7 +0,0 @@ -export default function JobMonitorLayout({ - children, -}: { - children: React.ReactNode; -}) { - return
{children}
; -} diff --git a/src/app/(dashboard)/jobmonitor/page.tsx b/src/app/(dashboard)/jobmonitor/page.tsx deleted file mode 100644 index 54fc0ed6..00000000 --- a/src/app/(dashboard)/jobmonitor/page.tsx +++ /dev/null @@ -1,5 +0,0 @@ -import JobMonitor from "@/components/applications/JobMonitor"; - -export default async function Page() { - return ; -} diff --git a/src/app/(dashboard)/layout.tsx b/src/app/(dashboard)/layout.tsx index 16520780..eb79c070 100644 --- a/src/app/(dashboard)/layout.tsx +++ b/src/app/(dashboard)/layout.tsx @@ -8,7 +8,7 @@ import Dashboard from "@/components/layout/Dashboard"; import ApplicationsProvider from "@/contexts/ApplicationsProvider"; import { OIDCSecure } from "@/components/layout/OIDCSecure"; -export default function JobMonitorLayout({ +export default function DashboardLayout({ children, }: { children: React.ReactNode; diff --git a/src/app/(dashboard)/page.tsx b/src/app/(dashboard)/page.tsx index 3967341a..ed193572 100644 --- a/src/app/(dashboard)/page.tsx +++ b/src/app/(dashboard)/page.tsx @@ -10,12 +10,16 @@ export default function Page() { const appId = searchParams.get("appId"); const [sections] = React.useContext(ApplicationsContext); - const appType = sections - .find((section) => section.items.some((item) => item.id === appId)) - ?.items.find((item) => item.id === appId)?.type; + const appType = React.useMemo(() => { + const section = sections.find((section) => + section.items.some((item) => item.id === appId), + ); + return section?.items.find((item) => item.id === appId)?.type; + }, [sections, appId]); - const Component = applicationList.find((app) => app.name === appType) - ?.component; + const Component = React.useMemo(() => { + return applicationList.find((app) => app.name === appType)?.component; + }, [appType]); return Component ? : ; } diff --git a/src/components/applications/ApplicationList.ts b/src/components/applications/ApplicationList.ts index 32d26fe5..7bda2436 100644 --- a/src/components/applications/ApplicationList.ts +++ b/src/components/applications/ApplicationList.ts @@ -1,8 +1,9 @@ import { Dashboard, FolderCopy, Monitor } from "@mui/icons-material"; import JobMonitor from "./JobMonitor"; import UserDashboard from "./UserDashboard"; +import ApplicationConfig from "@/types/ApplicationConfig"; -export const applicationList = [ +export const applicationList: ApplicationConfig[] = [ { name: "Dashboard", component: UserDashboard, icon: Dashboard }, { name: "Job Monitor", diff --git a/src/components/layout/OIDCSecure.tsx b/src/components/layout/OIDCSecure.tsx index 515ebd5c..bc6f9adb 100644 --- a/src/components/layout/OIDCSecure.tsx +++ b/src/components/layout/OIDCSecure.tsx @@ -23,6 +23,7 @@ export function OIDCSecure({ children }: OIDCProps) { if (!isAuthenticated) { router.push( "/auth?" + + // URLSearchParams to ensure that auth redirects users to the URL they came from new URLSearchParams({ redirect: window.location.href }).toString(), ); } diff --git a/src/components/ui/ApplicationDialog.tsx b/src/components/ui/ApplicationDialog.tsx index 0d15dddc..58864845 100644 --- a/src/components/ui/ApplicationDialog.tsx +++ b/src/components/ui/ApplicationDialog.tsx @@ -8,7 +8,9 @@ import { Button, Grid, Icon, + IconButton, } from "@mui/material"; +import { Close } from "@mui/icons-material"; import { applicationList } from "../applications/ApplicationList"; /** @@ -55,10 +57,26 @@ export default function AppDialog({ fullWidth maxWidth="sm" > - New Application + + Available applications + + setAppDialogOpen(false)} + sx={{ + position: "absolute", + right: 8, + top: 8, + color: (theme) => theme.palette.grey[500], + }} + > + + - Choose the type of application you would like to create. + Click on any application to open it in a new instance in the drawer. + Multiple instances of the same application can be opened + simultaneously. {applicationList.map((app) => ( @@ -78,14 +96,6 @@ export default function AppDialog({ ))} - - - ); } diff --git a/src/components/ui/DashboardDrawer.tsx b/src/components/ui/DashboardDrawer.tsx index 9ea65d65..637e36c8 100644 --- a/src/components/ui/DashboardDrawer.tsx +++ b/src/components/ui/DashboardDrawer.tsx @@ -1,4 +1,3 @@ -import { usePathname } from "next/navigation"; import { Box, Button, @@ -25,10 +24,11 @@ import React, { } from "react"; import { monitorForElements } from "@atlaskit/pragmatic-drag-and-drop/element/adapter"; import { extractClosestEdge } from "@atlaskit/pragmatic-drag-and-drop-hitbox/closest-edge"; +import Image from "next/image"; import DrawerItemGroup from "./DrawerItemGroup"; -import { DiracLogo } from "./DiracLogo"; import AppDialog from "./ApplicationDialog"; import { ApplicationsContext } from "@/contexts/ApplicationsProvider"; +import { useMUITheme } from "@/hooks/theme"; interface DashboardDrawerProps { variant: "permanent" | "temporary"; @@ -45,15 +45,12 @@ interface DashboardDrawerProps { * @returns {JSX.Element} The rendered DashboardDrawer component. */ export default function DashboardDrawer(props: DashboardDrawerProps) { - // Get the current URL - const pathname = usePathname(); // Determine the container for the Drawer based on whether the window object exists. const container = window !== undefined ? () => window.document.body : undefined; // Check if the drawer is in "temporary" mode. const isTemporary = props.variant === "temporary"; - - // Wether the modal for Application Creation is open + // Whether the modal for Application Creation is open const [appDialogOpen, setAppDialogOpen] = useState(false); const [contextMenu, setContextMenu] = React.useState<{ @@ -73,7 +70,10 @@ export default function DashboardDrawer(props: DashboardDrawerProps) { // Each section has an associated icon and path. const [userSections, setSections] = useContext(ApplicationsContext); + const theme = useMUITheme(); + useEffect(() => { + // Handle changes to sections when drag and drop occurs. return monitorForElements({ onDrop({ source, location }) { const target = location.current.dropTargets[0]; @@ -84,6 +84,7 @@ export default function DashboardDrawer(props: DashboardDrawerProps) { const targetData = target.data; if (location.current.dropTargets.length == 2) { + // If the target is an item const groupTitle = targetData.title; const closestEdgeOfTarget = extractClosestEdge(targetData); const targetIndex = targetData.index as number; @@ -105,6 +106,7 @@ export default function DashboardDrawer(props: DashboardDrawerProps) { destinationIndex, ); } else { + // If the target is a group const groupTitle = targetData.title; const sourceGroup = userSections.find( (group) => group.title == sourceData.title, @@ -140,13 +142,13 @@ export default function DashboardDrawer(props: DashboardDrawerProps) { destinationIndex && sourceIndex < destinationIndex ) { - destinationIndex -= 1; + destinationIndex -= 1; // Corrects the index within the same group if needed } if ( sourceGroup.title === destinationGroup.title && (destinationIndex == null || sourceIndex === destinationIndex) ) { - return; + return; // Nothing to do } if (sourceGroup.title === destinationGroup.title) { @@ -195,8 +197,7 @@ export default function DashboardDrawer(props: DashboardDrawerProps) { /** * Handles the creation of a new app in the dashboard drawer. * - * @param appName - The name of the app. - * @param path - The path of the app. + * @param appType - The type of the app to be created. * @param icon - The icon component for the app. */ const handleAppCreation = (appType: string, icon: ComponentType) => { @@ -361,8 +362,20 @@ export default function DashboardDrawer(props: DashboardDrawerProps) { style={{ display: "flex", flexDirection: "column", height: "100%" }} > {/* Display the logo in the toolbar section of the drawer. */} - - + + DIRAC logo {/* Map over user sections and render them as list items in the drawer. */} @@ -380,8 +393,17 @@ export default function DashboardDrawer(props: DashboardDrawerProps) { ))} - {/* Render a link to documentation, positioned at the bottom of the drawer. */} - + + {/* Render a link to documentation and a button to add applications, positioned at the bottom of the drawer. */} + setAppDialogOpen(true)}> diff --git a/src/components/ui/DiracLogo.tsx b/src/components/ui/DiracLogo.tsx deleted file mode 100644 index cff07b6f..00000000 --- a/src/components/ui/DiracLogo.tsx +++ /dev/null @@ -1,16 +0,0 @@ -import NextLink from "next/link"; -import Image from "next/image"; - -/** - * Logo of the DIRAC interware redirecting to the root page - * @returns a NextLink embedding an Image - */ -export function DiracLogo() { - return ( - <> - - DIRAC logo - - - ); -} diff --git a/src/components/ui/DrawerItem.tsx b/src/components/ui/DrawerItem.tsx index b2fff815..71f11bb8 100644 --- a/src/components/ui/DrawerItem.tsx +++ b/src/components/ui/DrawerItem.tsx @@ -1,6 +1,5 @@ import React, { useEffect, useState } from "react"; import { createRoot } from "react-dom/client"; -import Link from "next/link"; import { ListItemButton, ListItemIcon, @@ -25,6 +24,14 @@ import { ThemeProvider } from "@/contexts/ThemeProvider"; import { useMUITheme } from "@/hooks/theme"; import { useSearchParamsUtils } from "@/hooks/searchParamsUtils"; +/** + * Represents a drawer item component. + * + * @param item - The item object containing the title, id, and icon. + * @param index - The index of the item. + * @param groupTitle - The title of the group. + * @returns The rendered JSX for the drawer item. + */ export default function DrawerItem({ item: { title, id, icon }, index, @@ -34,11 +41,13 @@ export default function DrawerItem({ index: number; groupTitle: string; }) { + // Ref to use for the draggable element const dragRef = React.useRef(null); + // Ref to use for the handle of the draggable element, must be a child of the draggable element const handleRef = React.useRef(null); const theme = useMUITheme(); const { setParam } = useSearchParamsUtils(); - + // Represents the closest edge to the mouse cursor const [closestEdge, setClosestEdge]: any = useState(null); useEffect(() => { @@ -47,16 +56,21 @@ export default function DrawerItem({ const handleItem = handleRef.current; return combine( + // makes the item draggable draggable({ element: element, dragHandle: handleItem, + // Sets the initial data for the drag and drop interaction getInitialData: () => ({ index, title: groupTitle }), + // Sets a lightweight version of the real item as a preview onGenerateDragPreview: ({ nativeSetDragImage, source, location }) => { setCustomNativeDragPreview({ nativeSetDragImage, render: ({ container }) => { const root = createRoot(container); root.render( + // Wraps the preview in the theme provider to ensure the correct theme is applied + // This is necessary because the preview is rendered outside the main app
root.unmount(); }, + // Seamless transition between the preview and the real element getOffset: ({ container }) => { const elementPos = source.element.getBoundingClientRect(); const x = location.current.input.pageX - elementPos.x; @@ -80,6 +95,7 @@ export default function DrawerItem({ }); }, }), + // Makes the item a target for dragged elements. Attach the closest edge data and highlight the destination when hovering over the item dropTargetForElements({ element: element, getData: ({ input, element }) => { @@ -150,6 +166,15 @@ export default function DrawerItem({ ); } +/** + * Lightweight preview of an item in the drawer. + * Used when dragging an item to give a visual representation of it with minimal resources. + * + * @param {Object} props - The component props. + * @param {string} props.title - The title of the item. + * @param {React.ComponentType} props.icon - The icon component for the item. + * @returns {JSX.Element} The rendered item preview. + */ function ItemPreview({ title, icon, diff --git a/src/components/ui/DrawerItemGroup.tsx b/src/components/ui/DrawerItemGroup.tsx index 717caf83..f70f7fd8 100644 --- a/src/components/ui/DrawerItemGroup.tsx +++ b/src/components/ui/DrawerItemGroup.tsx @@ -8,37 +8,38 @@ import { UserSection } from "@/types/UserSection"; /** * Represents a group of items in a drawer. * - * @param group - The group object containing the title, expanded state, and items. - * @param setSections - The function to update the sections state. - * @returns The JSX element representing the drawer item group. + * @component + * @param {Object} props - The component props. + * @param {Object} props.group - The group object containing the title, expanded state, and items. + * @param {string} props.group.title - The title of the group. + * @param {boolean} props.group.extended - The expanded state of the group. + * @param {Array} props.group.items - The array of items in the group. + * @param {Function} props.setSections - The function to set the sections state. + * @param {Function} props.handleContextMenu - The function to handle the context menu. + * @returns {JSX.Element} The rendered DrawerItemGroup component. */ export default function DrawerItemGroup({ group: { title, extended: expanded, items }, setSections, handleContextMenu, }: { - group: { - title: string; - extended: boolean; - items: { - title: string; - id: string; - icon: React.ComponentType; - }[]; - }; + group: UserSection; setSections: React.Dispatch>; handleContextMenu: ( type: "group" | "item" | null, id: string | null, ) => (event: React.MouseEvent) => void; }) { + // Ref to use for the drag and drop target const dropRef = React.useRef(null); + // State to track whether the user is hovering over the item during a drag operation const [hovered, setHovered] = React.useState(false); useEffect(() => { if (!dropRef.current) return; const dropItem = dropRef.current; + // Makes the element a valid drop target, sets up the data transfer and manage the hovered state return dropTargetForElements({ element: dropItem, getData: () => ({ title }), @@ -52,6 +53,7 @@ export default function DrawerItemGroup({ }); }); + // Handles expansion of the accordion group const handleChange = (title: string) => (event: any, isExpanded: any) => { // Set the extended state of the accordion group. setSections((sections) => @@ -62,7 +64,6 @@ export default function DrawerItemGroup({ ), ); }; - const groupTitle = title; return ( {/* Accordion details */} - {items.map(({ title, id, icon }, index) => ( + {items.map(({ title: itemTitle, id, icon }, index) => (
))} diff --git a/src/contexts/ApplicationsProvider.tsx b/src/contexts/ApplicationsProvider.tsx index 9f9cc45b..2b346688 100644 --- a/src/contexts/ApplicationsProvider.tsx +++ b/src/contexts/ApplicationsProvider.tsx @@ -13,9 +13,7 @@ export const ApplicationsContext = createContext< >([[], () => {}]); // Create the ApplicationsProvider component -const ApplicationsProvider: React.FC<{ children: React.ReactNode }> = ({ - children, -}) => { +const ApplicationsProvider = ({ children }: { children: React.ReactNode }) => { const [userSections, setSections] = useState([]); const { getParam, setParam } = useSearchParamsUtils(); diff --git a/src/hooks/theme.tsx b/src/hooks/theme.tsx index 049d9bd1..03ce9a0d 100644 --- a/src/hooks/theme.tsx +++ b/src/hooks/theme.tsx @@ -36,6 +36,11 @@ export const useMUITheme = () => { }, }); + const scrollbarBackground = theme === "dark" ? "#333" : "#f1f1f1"; + const scrollbarThumbBackground = theme === "dark" ? "#888" : "#ccc"; + const scrollbarThumbHoverBackground = theme === "dark" ? "#555" : "#999"; + const scrollbarColor = `${scrollbarThumbBackground} ${scrollbarBackground}`; + muiTheme.components = { MuiCssBaseline: { styleOverrides: ` @@ -44,18 +49,18 @@ export const useMUITheme = () => { border-radius: 5px; } ::-webkit-scrollbar-track { - background: ${theme === "dark" ? "#333" : "#f1f1f1"}; + background: ${scrollbarBackground}; } ::-webkit-scrollbar-thumb { - background: ${theme === "dark" ? "#888" : "#ccc"}; + background: ${scrollbarThumbBackground}; border-radius: 5px; } ::-webkit-scrollbar-thumb:hover { - background: ${theme === "dark" ? "#555" : "#999"}; + background: ${scrollbarThumbHoverBackground}; } @supports not selector(::-webkit-scrollbar) { html { - scrollbar-color: ${theme === "dark" ? "#888 #333" : "#ccc #f1f1f1"}; + scrollbar-color: ${scrollbarColor}; } } `, diff --git a/src/types/ApplicationConfig.ts b/src/types/ApplicationConfig.ts new file mode 100644 index 00000000..324a53d8 --- /dev/null +++ b/src/types/ApplicationConfig.ts @@ -0,0 +1,8 @@ +import { SvgIconComponent } from "@mui/icons-material"; +import { ElementType } from "react"; + +export default interface ApplicationConfig { + name: string; + component: ElementType; + icon: SvgIconComponent; +} diff --git a/test/unit-tests/Dashboard.test.tsx b/test/unit-tests/Dashboard.test.tsx index 4df42844..f6551ffd 100644 --- a/test/unit-tests/Dashboard.test.tsx +++ b/test/unit-tests/Dashboard.test.tsx @@ -47,21 +47,19 @@ jest.mock("next/navigation", () => { }; }); -const MockThemeProvider: React.FC<{ children: React.ReactNode }> = ({ +const MockApplicationProvider: React.FC<{ children: React.ReactNode }> = ({ children, }): JSX.Element => ( - - { - mockSections = test(); - }), - ]} - > - {children} - - + { + mockSections = test(); + }), + ]} + > + {children} + ); describe("", () => { @@ -126,9 +124,11 @@ describe("", () => { describe("", () => { it("renders correctly", () => { const { getByText } = render( - - - , + + + + + , ); expect(getByText("App 1 Icon")).toBeInTheDocument(); @@ -136,9 +136,11 @@ describe("", () => { it("handles context menu", () => { const { getByText, getByTestId } = render( - - - , + + + + + , ); fireEvent.contextMenu(getByText("App 1")); diff --git a/test/unit-tests/DiracLogo.test.tsx b/test/unit-tests/DiracLogo.test.tsx deleted file mode 100644 index 9f2a9985..00000000 --- a/test/unit-tests/DiracLogo.test.tsx +++ /dev/null @@ -1,22 +0,0 @@ -import React from "react"; -import { render } from "@testing-library/react"; -import { DiracLogo } from "@/components/ui/DiracLogo"; - -describe("", () => { - it("renders the logo image with correct attributes", () => { - const { getByAltText } = render(); - - // Check if the image is rendered with the correct alt text - const logoImage = getByAltText("DIRAC logo"); - expect(logoImage).toBeInTheDocument(); - }); - - it("renders the link that redirects to the root page", () => { - const { getByRole } = render(); - - // Check if the link is rendered and points to the root page - const link = getByRole("link"); - expect(link).toBeInTheDocument(); - expect(link).toHaveAttribute("href", "/"); - }); -});