Skip to content

feat(site): move resources into the sidebar #11456

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 8, 2024
Merged

feat(site): move resources into the sidebar #11456

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eb1b7d7
Move resources to the sidebar
BrunoQuaresma Jan 5, 2024
4dace52
Assign review comments
BrunoQuaresma Jan 8, 2024
5c7ca86
Avoid sort mutation
BrunoQuaresma Jan 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Move resources to the sidebar
  • Loading branch information
@BrunoQuaresma
BrunoQuaresma committed Jan 5, 2024
commit eb1b7d771ad10dcd04fdbc151e8aa661385b1821
23 changes: 21 additions & 2 deletions site/src/components/FullPageLayout/Sidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,26 @@ export const SidebarLink = (props: LinkProps) => {
return <Link css={styles.sidebarItem} {...props} />;
};

export const SidebarItem = (props: HTMLAttributes<HTMLButtonElement>) => {
return <button css={styles.sidebarItem} {...props} />;
export const SidebarItem = (
props: HTMLAttributes<HTMLButtonElement> & { isActive?: boolean },
) => {
const { isActive, ...buttonProps } = props;
const theme = useTheme();

return (
<button
css={[
styles.sidebarItem,
{ opacity: "0.75", "&:hover": { opacity: 1 } },
isActive && {
background: theme.palette.action.selected,
opacity: 1,
pointerEvents: "none",
},
]}
{...buttonProps}
/>
);
};

export const SidebarCaption = (props: HTMLAttributes<HTMLSpanElement>) => {
Expand Down Expand Up @@ -88,6 +106,7 @@ const styles = {
textAlign: "left",
background: "none",
border: 0,
cursor: "pointer",

"&:hover": {
backgroundColor: theme.palette.action.hover,
Expand Down
24 changes: 2 additions & 22 deletions site/src/components/Resources/ResourceAvatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,32 +1,12 @@
import { type FC } from "react";
import type { WorkspaceResource } from "api/typesGenerated";
import { Avatar, AvatarIcon } from "components/Avatar/Avatar";

const FALLBACK_ICON = "/icon/widgets.svg";

// These resources (i.e. docker_image, kubernetes_deployment) map to Terraform
// resource types. These are the most used ones and are based on user usage.
// We may want to update from time-to-time.
const BUILT_IN_ICON_PATHS: Record<string, string> = {
docker_volume: "/icon/database.svg",
docker_container: "/icon/memory.svg",
docker_image: "/icon/container.svg",
kubernetes_persistent_volume_claim: "/icon/database.svg",
kubernetes_pod: "/icon/memory.svg",
google_compute_disk: "/icon/database.svg",
google_compute_instance: "/icon/memory.svg",
aws_instance: "/icon/memory.svg",
kubernetes_deployment: "/icon/memory.svg",
};

export const getIconPathResource = (resourceType: string): string => {
return BUILT_IN_ICON_PATHS[resourceType] ?? FALLBACK_ICON;
};
import { getResourceIconPath } from "utils/workspace";

export type ResourceAvatarProps = { resource: WorkspaceResource };

export const ResourceAvatar: FC<ResourceAvatarProps> = ({ resource }) => {
const avatarSrc = resource.icon || getIconPathResource(resource.type);
const avatarSrc = resource.icon || getResourceIconPath(resource.type);

return (
<Avatar background>
Expand Down
12 changes: 1 addition & 11 deletions site/src/components/Resources/ResourceCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,7 @@ const styles = {
resourceCard: (theme) => ({
borderRadius: 8,
border: `1px solid ${theme.palette.divider}`,

"&:not(:first-of-type)": {
borderTop: 0,
borderTopLeftRadius: 0,
borderTopRightRadius: 0,
},

"&:not(:last-child)": {
borderBottomLeftRadius: 0,
borderBottomRightRadius: 0,
},
background: theme.palette.background.default,
}),

resourceCardProfile: {
Expand Down
3 changes: 1 addition & 2 deletions site/src/components/WorkspaceBuild/WorkspaceBuildData.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ export const WorkspaceBuildData = ({ build }: { build: WorkspaceBuild }) => {
css={{
fontSize: 12,
color: theme.palette.text.secondary,
marginTop: 2,
}}
>
{createDayString(build.created_at)}
Expand Down Expand Up @@ -74,6 +73,6 @@ const styles = {
flexDirection: "row",
alignItems: "center",
gap: 12,
lineHeight: "1.4",
lineHeight: "1.5",
},
} satisfies Record<string, Interpolation<Theme>>;
109 changes: 109 additions & 0 deletions site/src/pages/WorkspacePage/ResourcesSidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
import { Interpolation, Theme } from "@emotion/react";
import Skeleton from "@mui/material/Skeleton";
import { useTheme } from "@mui/material/styles";
import { WorkspaceResource } from "api/typesGenerated";
import {
Sidebar,
SidebarCaption,
SidebarItem,
} from "components/FullPageLayout/Sidebar";
import { getResourceIconPath } from "utils/workspace";

type ResourcesSidebarProps = {
failed: boolean;
resources: WorkspaceResource[];
onChange: (resourceId: string) => void;
selected: string;
};

export const ResourcesSidebar = (props: ResourcesSidebarProps) => {
const theme = useTheme();
const { failed, onChange, selected, resources } = props;

return (
<Sidebar>
<SidebarCaption>Resources</SidebarCaption>
{failed && (
<p
css={{
margin: 0,
padding: "0 16px",
fontSize: 13,
color: theme.palette.text.secondary,
lineHeight: "1.5",
}}
>
Your workspace build failed, so the necessary resources couldn&apos;t
be created.
</p>
)}
{resources.length === 0 &&
!failed &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this condition strictly for handling loading states?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Array.from({ length: 8 }, (_, i) => (
<SidebarItem key={i}>
<ResourceSidebarItemSkeleton />
</SidebarItem>
))}
{resources.map((r) => (
<SidebarItem
onClick={() => onChange(r.id)}
isActive={r.id === selected}
key={r.id}
css={styles.root}
>
<div
css={{
display: "flex",
alignItems: "center",
justifyContent: "center",
lineHeight: 0,
Copy link
Member

@Parkreiner Parkreiner Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want a lineHeight of 0, or a lineHeight of 1? I've had issues where a value of 0 broke a lot of text rendering and made text invisible the moment something accidentally overflowed to a new line

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 1 🤔 but let me check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh in this case, it is an image wrapper so it is ok to use 0... I'm going to replace those wrappers - we have a few in the codebase - with the new @aslilac component ExternalIcon

width: 16,
height: 16,
padding: 2,
}}
>
<img
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add role="presentation" here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeap.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty alt has similar behavior to the presentation role.

In these cases, a null (empty) alt text should be provided (alt="") so that they can be ignored by assistive technologies, such as screen readers. Text values for these types of images would add audible clutter to screen reader output or could distract users if the topic is different from that in adjacent text. Leaving out the alt attribute is also not an option because when it is not provided, some screen readers will announce the file name of the image instead.

https://www.w3.org/WAI/tutorials/images/decorative/

css={{ width: "100%", height: "100%", objectFit: "contain" }}
src={getResourceIconPath(r.type)}
alt=""
/>
</div>
<div
css={{ display: "flex", flexDirection: "column", fontWeight: 500 }}
>
<span>{r.name}</span>
<span css={{ fontSize: 12, color: theme.palette.text.secondary }}>
{r.type}
</span>
</div>
</SidebarItem>
))}
</Sidebar>
);
};

export const ResourceSidebarItemSkeleton = () => {
return (
<div css={[styles.root, { pointerEvents: "none" }]}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does pointerEvents do anything here? Divs aren't interactive by default, and turning on the style rule would just make clicks on the div hit the parent directly, which wouldn't be that different from the events firing on the div, and then bubbling up to the parent

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is because it has the root style which applies a "hover" style. I just found it easier to set pointerEvents to none to remove that. Do you see better ways?

<Skeleton variant="circular" width={16} height={16} />
<div>
<Skeleton variant="text" width={94} height={16} />
<Skeleton
variant="text"
width={60}
height={14}
css={{ marginTop: 2 }}
/>
</div>
</div>
);
};

const styles = {
root: {
lineHeight: "1.5",
display: "flex",
alignItems: "center",
gap: 12,
},
} satisfies Record<string, Interpolation<Theme>>;
6 changes: 0 additions & 6 deletions site/src/pages/WorkspacePage/Workspace.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,6 @@ export const Running: Story = {
workspace: Mocks.MockWorkspace,
handleStart: action("start"),
handleStop: action("stop"),
resources: [
Mocks.MockWorkspaceResourceMultipleAgents,
Mocks.MockWorkspaceVolumeResource,
Mocks.MockWorkspaceImageResource,
Mocks.MockWorkspaceContainerResource,
],
canUpdateWorkspace: true,
workspaceErrors: {},
buildInfo: Mocks.MockBuildInfo,
Expand Down
77 changes: 57 additions & 20 deletions site/src/pages/WorkspacePage/Workspace.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,15 @@ import { type Interpolation, type Theme } from "@emotion/react";
import Button from "@mui/material/Button";
import AlertTitle from "@mui/material/AlertTitle";
import { type FC, useEffect, useState } from "react";
import { useNavigate, useSearchParams } from "react-router-dom";
import { useNavigate } from "react-router-dom";
import dayjs from "dayjs";
import type * as TypesGen from "api/typesGenerated";
import { Alert, AlertDetail } from "components/Alert/Alert";
import { Resources } from "components/Resources/Resources";
import { Stack } from "components/Stack/Stack";
import { ErrorAlert } from "components/Alert/ErrorAlert";
import { DormantWorkspaceBanner } from "components/WorkspaceDeletion";
import { AgentRow } from "components/Resources/AgentRow";
import { useLocalStorage } from "hooks";
import { useLocalStorage, useTab } from "hooks";
import {
ActiveTransition,
WorkspaceBuildProgress,
Expand All @@ -24,6 +23,9 @@ import { bannerHeight } from "components/Dashboard/DeploymentBanner/DeploymentBa
import HistoryOutlined from "@mui/icons-material/HistoryOutlined";
import { useTheme } from "@mui/material/styles";
import { SidebarIconButton } from "components/FullPageLayout/Sidebar";
import HubOutlined from "@mui/icons-material/HubOutlined";
import { ResourcesSidebar } from "./ResourcesSidebar";
import { ResourceCard } from "components/Resources/ResourceCard";

export type WorkspaceError =
| "getBuildsError"
Expand All @@ -45,7 +47,6 @@ export interface WorkspaceProps {
isUpdating: boolean;
isRestarting: boolean;
workspace: TypesGen.Workspace;
resources?: TypesGen.WorkspaceResource[];
canUpdateWorkspace: boolean;
updateMessage?: string;
canChangeVersions: boolean;
Expand Down Expand Up @@ -78,8 +79,6 @@ export const Workspace: FC<React.PropsWithChildren<WorkspaceProps>> = ({
workspace,
isUpdating,
isRestarting,
resources,

canUpdateWorkspace,
updateMessage,
canChangeVersions,
Expand All @@ -99,8 +98,6 @@ export const Workspace: FC<React.PropsWithChildren<WorkspaceProps>> = ({
const { saveLocal, getLocal } = useLocalStorage();
const theme = useTheme();

const [searchParams, setSearchParams] = useSearchParams();

const [showAlertPendingInQueue, setShowAlertPendingInQueue] = useState(false);

// 2023-11-15 - MES - This effect will be called every single render because
Expand Down Expand Up @@ -148,6 +145,29 @@ export const Workspace: FC<React.PropsWithChildren<WorkspaceProps>> = ({
const transitionStats =
template !== undefined ? ActiveTransition(template, workspace) : undefined;

const sidebarOption = useTab("sidebar", "");
const setSidebarOption = (newOption: string) => {
const { set, value } = sidebarOption;
if (value === newOption) {
set("");
} else {
set(newOption);
}
};

const selectedResourceId = useTab("resources", "");
const resources = workspace.latest_build.resources.sort(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accidental mutation here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I got it 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the .sort method is mutating the underlying array, even though that array is globally available throughout the app through the query cache. That could cause bugs for any other components that need the data in its original form

I don't think the .toSorted method is supported enough to swap that in, but you should be able to rewrite this as

 const resources = [...workspace.latest_build.resources].sort()

(a, b) => countAgents(b) - countAgents(a),
);
const selectedResource = workspace.latest_build.resources.find(
(r) => r.id === selectedResourceId.value,
);
useEffect(() => {
if (resources.length > 0 && selectedResourceId.value === "") {
selectedResourceId.set(resources[0].id);
}
}, [resources, selectedResourceId]);

return (
<div
css={{
Expand Down Expand Up @@ -187,25 +207,37 @@ export const Workspace: FC<React.PropsWithChildren<WorkspaceProps>> = ({
height: "100%",
overflowY: "auto",
borderRight: `1px solid ${theme.palette.divider}`,
display: "flex",
flexDirection: "column",
}}
>
<SidebarIconButton
isActive={searchParams.get("sidebar") === "history"}
isActive={sidebarOption.value === "resources"}
onClick={() => {
const sidebarOption = searchParams.get("sidebar");
if (sidebarOption === "history") {
searchParams.delete("sidebar");
} else {
searchParams.set("sidebar", "history");
}
setSearchParams(searchParams);
setSidebarOption("resources");
}}
>
<HubOutlined />
</SidebarIconButton>
<SidebarIconButton
isActive={sidebarOption.value === "history"}
onClick={() => {
setSidebarOption("history");
}}
>
<HistoryOutlined />
</SidebarIconButton>
</div>

{searchParams.get("sidebar") === "history" && (
{sidebarOption.value === "resources" && (
<ResourcesSidebar
failed={workspace.latest_build.status === "failed"}
resources={resources}
selected={selectedResourceId.value}
onChange={selectedResourceId.set}
/>
)}
{sidebarOption.value === "history" && (
<HistorySidebar workspace={workspace} />
)}

Expand Down Expand Up @@ -342,9 +374,9 @@ export const Workspace: FC<React.PropsWithChildren<WorkspaceProps>> = ({

{buildLogs}

{resources && resources.length > 0 && (
<Resources
resources={resources}
{selectedResource && (
<ResourceCard
resource={selectedResource}
agentRow={(agent) => (
<AgentRow
key={agent.id}
Expand All @@ -369,6 +401,10 @@ export const Workspace: FC<React.PropsWithChildren<WorkspaceProps>> = ({
);
};

const countAgents = (resource: TypesGen.WorkspaceResource) => {
return resource.agents ? resource.agents.length : 0;
};

const styles = {
content: {
padding: 24,
Expand All @@ -377,6 +413,7 @@ const styles = {
},

dotBackground: (theme) => ({
minHeight: "100%",
padding: 24,
"--d": "1px",
background: `
Expand Down
Loading