From 738b1b867c4c1b43368854ea2f38d2d1db176a70 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Thu, 20 Jun 2024 17:11:16 +0000 Subject: [PATCH 1/2] chore(site): refactor popover to make it easier to extend --- site/src/components/IconField/IconField.tsx | 48 +++-- site/src/components/Popover/Popover.tsx | 79 ++++----- .../dashboard/Navbar/ProxyMenu.stories.tsx | 7 +- .../Navbar/UserDropdown/UserDropdown.tsx | 77 ++++---- .../UserDropdown/UserDropdownContent.tsx | 2 +- .../resources/SSHButton/SSHButton.stories.tsx | 9 +- .../modules/resources/SSHButton/SSHButton.tsx | 4 +- .../TemplateInsightsPage/DateRange.tsx | 165 +++++++++--------- .../UsersTable/EditRolesButton.stories.tsx | 24 +-- .../UsersPage/UsersTable/EditRolesButton.tsx | 4 +- .../DownloadLogsDialog.stories.tsx | 9 +- .../WorkspaceActions.stories.tsx | 9 +- site/src/testHelpers/storybook.tsx | 6 + 13 files changed, 211 insertions(+), 232 deletions(-) diff --git a/site/src/components/IconField/IconField.tsx b/site/src/components/IconField/IconField.tsx index b2bf348d4d532..3690a78983195 100644 --- a/site/src/components/IconField/IconField.tsx +++ b/site/src/components/IconField/IconField.tsx @@ -3,7 +3,7 @@ import Button from "@mui/material/Button"; import InputAdornment from "@mui/material/InputAdornment"; import TextField, { type TextFieldProps } from "@mui/material/TextField"; import { visuallyHidden } from "@mui/utils"; -import { type FC, lazy, Suspense } from "react"; +import { type FC, lazy, Suspense, useState } from "react"; import { DropdownArrow } from "components/DropdownArrow/DropdownArrow"; import { ExternalImage } from "components/ExternalImage/ExternalImage"; import { Loader } from "components/Loader/Loader"; @@ -37,6 +37,7 @@ export const IconField: FC = ({ const theme = useTheme(); const hasIcon = textFieldProps.value && textFieldProps.value !== ""; + const [open, setOpen] = useState(false); return ( @@ -86,31 +87,26 @@ export const IconField: FC = ({ } `} /> - - {(popover) => ( - <> - - - - - }> - { - const value = - emoji.src ?? urlFromUnifiedCode(emoji.unified); - onPickEmoji(value); - popover.setIsOpen(false); - }} - /> - - - - )} + + + + + + }> + { + const value = emoji.src ?? urlFromUnifiedCode(emoji.unified); + onPickEmoji(value); + setOpen(false); + }} + /> + + {/* diff --git a/site/src/components/Popover/Popover.tsx b/site/src/components/Popover/Popover.tsx index 1ed4e90a73995..3793c7581c99c 100644 --- a/site/src/components/Popover/Popover.tsx +++ b/site/src/components/Popover/Popover.tsx @@ -12,7 +12,6 @@ import { type ReactNode, type RefObject, useContext, - useEffect, useId, useRef, useState, @@ -25,14 +24,12 @@ type TriggerRef = RefObject; type TriggerElement = ReactElement<{ ref: TriggerRef; onClick?: () => void; - "aria-haspopup"?: boolean; - "aria-owns"?: string | undefined; }>; type PopoverContextValue = { id: string; - isOpen: boolean; - setIsOpen: React.Dispatch>; + open: boolean; + setOpen: (open: boolean) => void; triggerRef: TriggerRef; mode: TriggerMode; }; @@ -41,32 +38,40 @@ const PopoverContext = createContext( undefined, ); -export interface PopoverProps { - children: ReactNode | ((popover: PopoverContextValue) => ReactNode); // Allows inline usage +type BasePopoverProps = { + children: ReactNode; mode?: TriggerMode; - isDefaultOpen?: boolean; -} +}; -export const Popover: FC = ({ - children, - mode, - isDefaultOpen, -}) => { +type UncontrolledPopoverProps = BasePopoverProps & { + open?: undefined; + onOpenChange?: undefined; +}; + +type ControlledPopoverProps = BasePopoverProps & { + open: boolean; + onOpenChange: (open: boolean) => void; +}; + +export type PopoverProps = UncontrolledPopoverProps | ControlledPopoverProps; + +export const Popover: FC = (props) => { const hookId = useId(); - const [isOpen, setIsOpen] = useState(isDefaultOpen ?? false); - const triggerRef = useRef(null); + const [open, setOpen] = useState(false); + const triggerRef: TriggerRef = useRef(null); + const isControlled = props.open !== undefined; const value: PopoverContextValue = { - isOpen, - setIsOpen, triggerRef, id: `${hookId}-popover`, - mode: mode ?? "click", + mode: props.mode ?? "click", + open: isControlled ? props.open : open, + setOpen: isControlled ? props.onOpenChange : setOpen, }; return ( - {typeof children === "function" ? children(value) : children} + {props.children} ); }; @@ -82,23 +87,25 @@ export const usePopover = () => { }; export const PopoverTrigger = ( - props: HTMLAttributes & { children: TriggerElement }, + props: HTMLAttributes & { + children: TriggerElement; + }, ) => { const popover = usePopover(); const { children, ...elementProps } = props; const clickProps = { onClick: () => { - popover.setIsOpen((isOpen) => !isOpen); + popover.setOpen(true); }, }; const hoverProps = { onPointerEnter: () => { - popover.setIsOpen(true); + popover.setOpen(true); }, onPointerLeave: () => { - popover.setIsOpen(false); + popover.setOpen(false); }, }; @@ -106,7 +113,7 @@ export const PopoverTrigger = ( ...elementProps, ...(popover.mode === "click" ? clickProps : hoverProps), "aria-haspopup": true, - "aria-owns": popover.isOpen ? popover.id : undefined, + "aria-owns": popover.open ? popover.id : undefined, ref: popover.triggerRef, }); }; @@ -125,22 +132,8 @@ export const PopoverContent: FC = ({ ...popoverProps }) => { const popover = usePopover(); - const [isReady, setIsReady] = useState(false); const hoverMode = popover.mode === "hover"; - // This is a hack to make sure the popover is not rendered until the trigger - // is ready. This is a limitation on MUI that does not support defaultIsOpen - // on Popover but we need it to storybook the component. - useEffect(() => { - if (!isReady && popover.triggerRef.current !== null) { - setIsReady(true); - } - }, [isReady, popover.triggerRef]); - - if (!popover.triggerRef.current) { - return null; - } - return ( = ({ {...modeProps(popover)} {...popoverProps} id={popover.id} - open={popover.isOpen} - onClose={() => popover.setIsOpen(false)} + open={popover.open} + onClose={() => popover.setOpen(false)} anchorEl={popover.triggerRef.current} /> ); @@ -172,10 +165,10 @@ const modeProps = (popover: PopoverContextValue) => { if (popover.mode === "hover") { return { onPointerEnter: () => { - popover.setIsOpen(true); + popover.setOpen(true); }, onPointerLeave: () => { - popover.setIsOpen(false); + popover.setOpen(false); }, }; } diff --git a/site/src/modules/dashboard/Navbar/ProxyMenu.stories.tsx b/site/src/modules/dashboard/Navbar/ProxyMenu.stories.tsx index 185677b62d8df..0780f8eead76e 100644 --- a/site/src/modules/dashboard/Navbar/ProxyMenu.stories.tsx +++ b/site/src/modules/dashboard/Navbar/ProxyMenu.stories.tsx @@ -11,6 +11,7 @@ import { MockUser, MockWorkspaceProxies, } from "testHelpers/entities"; +import { withDesktopViewport } from "testHelpers/storybook"; import { ProxyMenu } from "./ProxyMenu"; const defaultProxyContextValue = { @@ -36,11 +37,7 @@ const meta: Meta = { ), - (Story) => ( -
- -
- ), + withDesktopViewport, ], parameters: { queries: [ diff --git a/site/src/modules/dashboard/Navbar/UserDropdown/UserDropdown.tsx b/site/src/modules/dashboard/Navbar/UserDropdown/UserDropdown.tsx index b42858157d142..da5a56d9cd12f 100644 --- a/site/src/modules/dashboard/Navbar/UserDropdown/UserDropdown.tsx +++ b/site/src/modules/dashboard/Navbar/UserDropdown/UserDropdown.tsx @@ -1,6 +1,6 @@ import { css, type Interpolation, type Theme, useTheme } from "@emotion/react"; import Badge from "@mui/material/Badge"; -import type { FC } from "react"; +import { useState, type FC } from "react"; import type * as TypesGen from "api/typesGenerated"; import { DropdownArrow } from "components/DropdownArrow/DropdownArrow"; import { @@ -26,48 +26,45 @@ export const UserDropdown: FC = ({ onSignOut, }) => { const theme = useTheme(); + const [open, setOpen] = useState(false); return ( - - {(popover) => ( - <> - - - - - - + + + + + + + ); }; diff --git a/site/src/modules/dashboard/Navbar/UserDropdown/UserDropdownContent.tsx b/site/src/modules/dashboard/Navbar/UserDropdown/UserDropdownContent.tsx index b8766698d4ca7..b82178b3812fc 100644 --- a/site/src/modules/dashboard/Navbar/UserDropdown/UserDropdownContent.tsx +++ b/site/src/modules/dashboard/Navbar/UserDropdown/UserDropdownContent.tsx @@ -43,7 +43,7 @@ export const UserDropdownContent: FC = ({ const popover = usePopover(); const onPopoverClose = () => { - popover.setIsOpen(false); + popover.setOpen(false); }; const renderMenuIcon = (icon: string): JSX.Element => { diff --git a/site/src/modules/resources/SSHButton/SSHButton.stories.tsx b/site/src/modules/resources/SSHButton/SSHButton.stories.tsx index a27e5102746ca..9b4e48759dd51 100644 --- a/site/src/modules/resources/SSHButton/SSHButton.stories.tsx +++ b/site/src/modules/resources/SSHButton/SSHButton.stories.tsx @@ -1,5 +1,7 @@ import type { Meta, StoryObj } from "@storybook/react"; +import { userEvent, within } from "@storybook/test"; import { MockWorkspace, MockWorkspaceAgent } from "testHelpers/entities"; +import { withDesktopViewport } from "testHelpers/storybook"; import { SSHButton } from "./SSHButton"; const meta: Meta = { @@ -22,7 +24,12 @@ export const Opened: Story = { args: { workspaceName: MockWorkspace.name, agentName: MockWorkspaceAgent.name, - isDefaultOpen: true, sshPrefix: "coder.", }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const button = canvas.getByRole("button"); + await userEvent.click(button); + }, + decorators: [withDesktopViewport], }; diff --git a/site/src/modules/resources/SSHButton/SSHButton.tsx b/site/src/modules/resources/SSHButton/SSHButton.tsx index d4b8662705eab..fe70ad05253d2 100644 --- a/site/src/modules/resources/SSHButton/SSHButton.tsx +++ b/site/src/modules/resources/SSHButton/SSHButton.tsx @@ -20,20 +20,18 @@ import { docs } from "utils/docs"; export interface SSHButtonProps { workspaceName: string; agentName: string; - isDefaultOpen?: boolean; sshPrefix?: string; } export const SSHButton: FC = ({ workspaceName, agentName, - isDefaultOpen = false, sshPrefix, }) => { const paper = useClassName(classNames.paper, []); return ( - + - - - { - const range = item.selection; - setRanges([range]); - - // When it is the first selection, we don't want to close the popover - // We have to do that ourselves because the library doesn't provide a way to do it - if (selectionStatusRef.current === "idle") { - selectionStatusRef.current = "selecting"; - return; - } - - selectionStatusRef.current = "idle"; - const startDate = range.startDate as Date; - const endDate = range.endDate as Date; - const now = new Date(); - onChange({ - startDate: startOfDay(startDate), - endDate: isToday(endDate) - ? startOfHour(addHours(now, 1)) - : startOfDay(addDays(endDate, 1)), - }); - popover.setIsOpen(false); - }} - moveRangeOnFirstSelection={false} - months={2} - ranges={ranges} - maxDate={new Date()} - direction="horizontal" - staticRanges={createStaticRanges([ - { - label: "Today", - range: () => ({ - startDate: new Date(), - endDate: new Date(), - }), - }, - { - label: "Yesterday", - range: () => ({ - startDate: subDays(new Date(), 1), - endDate: subDays(new Date(), 1), - }), - }, - { - label: "Last 7 days", - range: () => ({ - startDate: subDays(new Date(), 6), - endDate: new Date(), - }), - }, - { - label: "Last 14 days", - range: () => ({ - startDate: subDays(new Date(), 13), - endDate: new Date(), - }), - }, - { - label: "Last 30 days", - range: () => ({ - startDate: subDays(new Date(), 29), - endDate: new Date(), - }), - }, - ])} - /> - - - )} + + + + + + { + const range = item.selection; + setRanges([range]); + + // When it is the first selection, we don't want to close the popover + // We have to do that ourselves because the library doesn't provide a way to do it + if (selectionStatusRef.current === "idle") { + selectionStatusRef.current = "selecting"; + return; + } + + selectionStatusRef.current = "idle"; + const startDate = range.startDate as Date; + const endDate = range.endDate as Date; + const now = new Date(); + onChange({ + startDate: startOfDay(startDate), + endDate: isToday(endDate) + ? startOfHour(addHours(now, 1)) + : startOfDay(addDays(endDate, 1)), + }); + setOpen(false); + }} + moveRangeOnFirstSelection={false} + months={2} + ranges={ranges} + maxDate={new Date()} + direction="horizontal" + staticRanges={createStaticRanges([ + { + label: "Today", + range: () => ({ + startDate: new Date(), + endDate: new Date(), + }), + }, + { + label: "Yesterday", + range: () => ({ + startDate: subDays(new Date(), 1), + endDate: subDays(new Date(), 1), + }), + }, + { + label: "Last 7 days", + range: () => ({ + startDate: subDays(new Date(), 6), + endDate: new Date(), + }), + }, + { + label: "Last 14 days", + range: () => ({ + startDate: subDays(new Date(), 13), + endDate: new Date(), + }), + }, + { + label: "Last 30 days", + range: () => ({ + startDate: subDays(new Date(), 29), + endDate: new Date(), + }), + }, + ])} + /> + ); }; diff --git a/site/src/pages/UsersPage/UsersTable/EditRolesButton.stories.tsx b/site/src/pages/UsersPage/UsersTable/EditRolesButton.stories.tsx index 1a7ba3654cd55..34fc8fb9c82fc 100644 --- a/site/src/pages/UsersPage/UsersTable/EditRolesButton.stories.tsx +++ b/site/src/pages/UsersPage/UsersTable/EditRolesButton.stories.tsx @@ -1,43 +1,43 @@ import type { Meta, StoryObj } from "@storybook/react"; +import { userEvent, within } from "@storybook/test"; import { MockOwnerRole, MockSiteRoles, MockUserAdminRole, } from "testHelpers/entities"; +import { withDesktopViewport } from "testHelpers/storybook"; import { EditRolesButton } from "./EditRolesButton"; const meta: Meta = { title: "pages/UsersPage/EditRolesButton", component: EditRolesButton, args: { - isDefaultOpen: true, + selectedRoleNames: new Set([MockUserAdminRole.name, MockOwnerRole.name]), + roles: MockSiteRoles, }, + decorators: [withDesktopViewport], }; export default meta; type Story = StoryObj; -const selectedRoleNames = new Set([MockUserAdminRole.name, MockOwnerRole.name]); +export const Closed: Story = {}; export const Open: Story = { - args: { - selectedRoleNames, - roles: MockSiteRoles, - }, - parameters: { - chromatic: { delay: 300 }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.click(canvas.getByRole("button")); }, }; export const Loading: Story = { args: { isLoading: true, - selectedRoleNames, - roles: MockSiteRoles, userLoginType: "password", oidcRoleSync: false, }, - parameters: { - chromatic: { delay: 300 }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.click(canvas.getByRole("button")); }, }; diff --git a/site/src/pages/UsersPage/UsersTable/EditRolesButton.tsx b/site/src/pages/UsersPage/UsersTable/EditRolesButton.tsx index b304bbed01f89..82cc574fb13a1 100644 --- a/site/src/pages/UsersPage/UsersTable/EditRolesButton.tsx +++ b/site/src/pages/UsersPage/UsersTable/EditRolesButton.tsx @@ -72,7 +72,6 @@ export interface EditRolesButtonProps { roles: readonly SlimRole[]; selectedRoleNames: Set; onChange: (roles: SlimRole["name"][]) => void; - isDefaultOpen?: boolean; oidcRoleSync: boolean; userLoginType: string; } @@ -82,7 +81,6 @@ export const EditRolesButton: FC = ({ selectedRoleNames, onChange, isLoading, - isDefaultOpen = false, userLoginType, oidcRoleSync, }) => { @@ -116,7 +114,7 @@ export const EditRolesButton: FC = ({ } return ( - + = { @@ -24,13 +25,7 @@ const meta: Meta = { }, ], }, - decorators: [ - (Story) => ( -
- -
- ), - ], + decorators: [withDesktopViewport], }; export default meta; diff --git a/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.stories.tsx b/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.stories.tsx index e4188b7b88041..b42dbd418a04f 100644 --- a/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.stories.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.stories.tsx @@ -2,6 +2,7 @@ import type { Meta, StoryObj } from "@storybook/react"; import { userEvent, within, expect } from "@storybook/test"; import { buildLogsKey, agentLogsKey } from "api/queries/workspaces"; import * as Mocks from "testHelpers/entities"; +import { withDesktopViewport } from "testHelpers/storybook"; import { WorkspaceActions } from "./WorkspaceActions"; const meta: Meta = { @@ -10,13 +11,7 @@ const meta: Meta = { args: { isUpdating: false, }, - decorators: [ - (Story) => ( -
- -
- ), - ], + decorators: [withDesktopViewport], }; export default meta; diff --git a/site/src/testHelpers/storybook.tsx b/site/src/testHelpers/storybook.tsx index 77c0305d1aa2b..af1ba691bf826 100644 --- a/site/src/testHelpers/storybook.tsx +++ b/site/src/testHelpers/storybook.tsx @@ -78,3 +78,9 @@ export const withWebSocket = (Story: FC, { parameters }: StoryContext) => { return ; }; + +export const withDesktopViewport = (Story: FC) => ( +
+ +
+); From 26951f598aa0031e12bb1c54db0c8471c614a8a0 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Fri, 21 Jun 2024 14:02:30 +0000 Subject: [PATCH 2/2] Apply review suggestions and fix lint --- site/src/components/HelpTooltip/HelpTooltip.tsx | 2 +- site/src/components/Popover/Popover.tsx | 12 +++++++----- .../modules/dashboard/Navbar/DeploymentDropdown.tsx | 2 +- .../WorkspaceOutdatedTooltip.tsx | 2 +- .../WorkspaceActions/BuildParametersPopover.tsx | 2 +- .../WorkspaceNotifications/Notifications.tsx | 2 +- 6 files changed, 12 insertions(+), 10 deletions(-) diff --git a/site/src/components/HelpTooltip/HelpTooltip.tsx b/site/src/components/HelpTooltip/HelpTooltip.tsx index 31e2fe1fb260d..ffa4aa61fe6a3 100644 --- a/site/src/components/HelpTooltip/HelpTooltip.tsx +++ b/site/src/components/HelpTooltip/HelpTooltip.tsx @@ -160,7 +160,7 @@ export const HelpTooltipAction: FC = ({ onClick={(event) => { event.stopPropagation(); onClick(); - popover.setIsOpen(false); + popover.setOpen(false); }} > diff --git a/site/src/components/Popover/Popover.tsx b/site/src/components/Popover/Popover.tsx index 3793c7581c99c..ffb56fd5d7349 100644 --- a/site/src/components/Popover/Popover.tsx +++ b/site/src/components/Popover/Popover.tsx @@ -43,6 +43,8 @@ type BasePopoverProps = { mode?: TriggerMode; }; +// By separating controlled and uncontrolled props, we achieve more accurate +// type inference. type UncontrolledPopoverProps = BasePopoverProps & { open?: undefined; onOpenChange?: undefined; @@ -57,16 +59,15 @@ export type PopoverProps = UncontrolledPopoverProps | ControlledPopoverProps; export const Popover: FC = (props) => { const hookId = useId(); - const [open, setOpen] = useState(false); + const [uncontrolledOpen, setUncontrolledOpen] = useState(false); const triggerRef: TriggerRef = useRef(null); - const isControlled = props.open !== undefined; const value: PopoverContextValue = { triggerRef, id: `${hookId}-popover`, mode: props.mode ?? "click", - open: isControlled ? props.open : open, - setOpen: isControlled ? props.onOpenChange : setOpen, + open: props.open ?? uncontrolledOpen, + setOpen: props.onOpenChange ?? setUncontrolledOpen, }; return ( @@ -113,7 +114,8 @@ export const PopoverTrigger = ( ...elementProps, ...(popover.mode === "click" ? clickProps : hoverProps), "aria-haspopup": true, - "aria-owns": popover.open ? popover.id : undefined, + "aria-owns": popover.id, + "aria-expanded": popover.open, ref: popover.triggerRef, }); }; diff --git a/site/src/modules/dashboard/Navbar/DeploymentDropdown.tsx b/site/src/modules/dashboard/Navbar/DeploymentDropdown.tsx index e54210d831d8e..4a79bc00c1931 100644 --- a/site/src/modules/dashboard/Navbar/DeploymentDropdown.tsx +++ b/site/src/modules/dashboard/Navbar/DeploymentDropdown.tsx @@ -87,7 +87,7 @@ const DeploymentDropdownContent: FC = ({ }) => { const popover = usePopover(); - const onPopoverClose = () => popover.setIsOpen(false); + const onPopoverClose = () => popover.setOpen(false); return (