From 2956e06c786590a2e748c89fc31a1a57864a5a92 Mon Sep 17 00:00:00 2001 From: kylecarbs Date: Mon, 20 Jun 2022 16:34:14 +0000 Subject: [PATCH] fix: Wrap TableCell with Link for native browser routing Tables were previously using an onClick handler which replicated some Link behavior, but not natively through the browser. Fixes #2525. --- .../components/BuildsTable/BuildsTable.tsx | 29 +++++------- .../TableCellLink/TableCellLink.tsx | 47 +++++++++++++++++++ .../pages/TemplatesPage/TemplatesPageView.tsx | 28 +++++------ .../WorkspacesPage/WorkspacesPageView.tsx | 33 ++++++------- site/src/theme/overrides.ts | 6 ++- 5 files changed, 92 insertions(+), 51 deletions(-) create mode 100644 site/src/components/TableCellLink/TableCellLink.tsx diff --git a/site/src/components/BuildsTable/BuildsTable.tsx b/site/src/components/BuildsTable/BuildsTable.tsx index c9de9bd6d14ea..d2b09103b6cb4 100644 --- a/site/src/components/BuildsTable/BuildsTable.tsx +++ b/site/src/components/BuildsTable/BuildsTable.tsx @@ -12,6 +12,7 @@ import { useNavigate, useParams } from "react-router-dom" import * as TypesGen from "../../api/typesGenerated" import { displayWorkspaceBuildDuration, getDisplayWorkspaceBuildStatus } from "../../util/workspace" import { EmptyState } from "../EmptyState/EmptyState" +import { TableCellLink } from "../TableCellLink/TableCellLink" import { TableLoader } from "../TableLoader/TableLoader" export const Language = { @@ -51,10 +52,7 @@ export const BuildsTable: FC = ({ builds, className }) => { {builds && builds.map((build) => { const status = getDisplayWorkspaceBuildStatus(theme, build) - - const navigateToBuildPage = () => { - navigate(`/@${username}/${workspaceName}/builds/${build.build_number}`) - } + const buildPageLink = `/@${username}/${workspaceName}/builds/${build.build_number}` return ( = ({ builds, className }) => { key={build.id} data-testid={`build-${build.id}`} tabIndex={0} - onClick={navigateToBuildPage} onKeyDown={(event) => { if (event.key === "Enter") { - navigateToBuildPage() + navigate(buildPageLink) } }} className={styles.clickableTableRow} > - {build.transition} - + {build.transition} + {displayWorkspaceBuildDuration(build)} - - + + {new Date(build.created_at).toLocaleString()} - - + + {status.status} - - + +
- + ) })} @@ -107,8 +104,6 @@ export const BuildsTable: FC = ({ builds, className }) => { const useStyles = makeStyles((theme) => ({ clickableTableRow: { - cursor: "pointer", - "&:hover td": { backgroundColor: fade(theme.palette.primary.light, 0.1), }, diff --git a/site/src/components/TableCellLink/TableCellLink.tsx b/site/src/components/TableCellLink/TableCellLink.tsx new file mode 100644 index 0000000000000..ae23428bc8f18 --- /dev/null +++ b/site/src/components/TableCellLink/TableCellLink.tsx @@ -0,0 +1,47 @@ +import Link from "@material-ui/core/Link" +import { makeStyles } from "@material-ui/core/styles" +import TableCell, { TableCellProps } from "@material-ui/core/TableCell" +import { Link as RouterLink } from "react-router-dom" +import { combineClasses } from "../../util/combineClasses" + +// TableCellLink wraps a TableCell filling the entirety with a Link. +// This allows table rows to be clickable with browser-behavior like ctrl+click. +export const TableCellLink: React.FC< + TableCellProps & { + to: string + } +> = (props) => { + const styles = useStyles() + + return ( + + + {props.children} + + + ) +} + +const useStyles = makeStyles((theme) => ({ + cell: { + // This must override all padding for all rules on a TableCell. + // Otherwise, the link will not cover the entire region. + // It's unfortuante to use `!important`, but this seems to be + // a reasonable use-case. + padding: "0 !important", + }, + link: { + display: "block", + width: "100%", + border: "none", + background: "none", + paddingTop: theme.spacing(2), + paddingBottom: theme.spacing(2), + // This is required to hide all underlines for child elements! + textDecoration: "none !important", + }, +})) diff --git a/site/src/pages/TemplatesPage/TemplatesPageView.tsx b/site/src/pages/TemplatesPage/TemplatesPageView.tsx index bd691e3c7df66..8a58c763a326d 100644 --- a/site/src/pages/TemplatesPage/TemplatesPageView.tsx +++ b/site/src/pages/TemplatesPage/TemplatesPageView.tsx @@ -17,6 +17,7 @@ import { EmptyState } from "../../components/EmptyState/EmptyState" import { Margins } from "../../components/Margins/Margins" import { PageHeader, PageHeaderSubtitle, PageHeaderTitle } from "../../components/PageHeader/PageHeader" import { Stack } from "../../components/Stack/Stack" +import { TableCellLink } from "../../components/TableCellLink/TableCellLink" import { TableLoader } from "../../components/TableLoader/TableLoader" import { HelpTooltip, @@ -115,36 +116,37 @@ export const TemplatesPageView: FC = (props) => { )} {props.templates?.map((template) => { - const navigateToTemplatePage = () => { - navigate(`/templates/${template.name}`) - } + const templatePageLink = `/templates/${template.name}` return ( { if (event.key === "Enter") { - navigateToTemplatePage() + navigate(templatePageLink) } }} className={styles.clickableTableRow} > - + - + - {Language.developerCount(template.workspace_owner_count)} + + {Language.developerCount(template.workspace_owner_count)} + - {dayjs().to(dayjs(template.updated_at))} - {template.created_by_name} - + + {dayjs().to(dayjs(template.updated_at))} + + {template.created_by_name} +
- + ) })} @@ -159,8 +161,6 @@ const useStyles = makeStyles((theme) => ({ maxWidth: theme.spacing(62), }, clickableTableRow: { - cursor: "pointer", - "&:hover td": { backgroundColor: fade(theme.palette.primary.light, 0.1), }, diff --git a/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx b/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx index 113cd330b0a25..a66a58005e6e3 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPageView.tsx @@ -28,6 +28,7 @@ import { EmptyState } from "../../components/EmptyState/EmptyState" import { Margins } from "../../components/Margins/Margins" import { PageHeader, PageHeaderSubtitle, PageHeaderTitle } from "../../components/PageHeader/PageHeader" import { Stack } from "../../components/Stack/Stack" +import { TableCellLink } from "../../components/TableCellLink/TableCellLink" import { TableLoader } from "../../components/TableLoader/TableLoader" import { HelpTooltip, @@ -104,27 +105,25 @@ const WorkspaceRow: React.FC<{ workspaceRef: WorkspaceItemMachineRef }> = ({ wor const [workspaceState, send] = useActor(workspaceRef) const { data: workspace } = workspaceState.context const status = getDisplayStatus(theme, workspace.latest_build) - const navigateToWorkspacePage = () => { - navigate(`/@${workspace.owner_name}/${workspace.name}`) - } + const workspacePageLink = `/@${workspace.owner_name}/${workspace.name}` + return ( { if (event.key === "Enter") { - navigateToWorkspacePage() + navigate(workspacePageLink) } }} className={styles.clickableTableRow} > - + - - {workspace.template_name} - + + {workspace.template_name} + {workspace.outdated ? ( {Language.outdatedLabel} @@ -137,20 +136,20 @@ const WorkspaceRow: React.FC<{ workspaceRef: WorkspaceItemMachineRef }> = ({ wor ) : ( {Language.upToDateLabel} )} - - + + {dayjs().to(dayjs(workspace.latest_build.created_at))} - - + + {status.status} - - + +
- + ) } @@ -349,8 +348,6 @@ const useStyles = makeStyles((theme) => ({ }, }, clickableTableRow: { - cursor: "pointer", - "&:hover td": { backgroundColor: fade(theme.palette.primary.light, 0.1), }, diff --git a/site/src/theme/overrides.ts b/site/src/theme/overrides.ts index 803706bfeedc4..40b672cd6ca17 100644 --- a/site/src/theme/overrides.ts +++ b/site/src/theme/overrides.ts @@ -61,10 +61,12 @@ export const getOverrides = (palette: PaletteOptions) => { background: palette.background?.paper, borderBottom: `1px solid ${palette.divider}`, padding: 8, - "&:first-child": { + // This targets the first+last td elements, and also the first+last elements + // of a TableCellLink. + "&:not(:only-child):first-child, &:not(:only-child):first-child > a": { paddingLeft: 32, }, - "&:last-child": { + "&:not(:only-child):last-child, &:not(:only-child):last-child > a": { paddingRight: 32, }, },