Skip to content

refactor(site): refactor create workspace button #10303

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 1 commit into from
Oct 17, 2023
Merged

refactor(site): refactor create workspace button #10303

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
refactor(site): refactor create workspace button
  • Loading branch information
@BrunoQuaresma
BrunoQuaresma committed Oct 16, 2023
commit 3bbcd1c12df7825da8df3d46713581b45b731db9
351 changes: 186 additions & 165 deletions site/src/pages/WorkspacesPage/WorkspacesButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,121 +1,30 @@
import { type PropsWithChildren, type ReactNode, useState } from "react";
import { useTheme } from "@emotion/react";
import { Language } from "./WorkspacesPageView";

import {
type PropsWithChildren,
type ReactNode,
useState,
useRef,
} from "react";
import { type Template } from "api/typesGenerated";
import { type UseQueryResult } from "react-query";

import { Link as RouterLink } from "react-router-dom";
import {
Link as RouterLink,
LinkProps as RouterLinkProps,
} from "react-router-dom";
import Box from "@mui/system/Box";
import Button from "@mui/material/Button";
import Link from "@mui/material/Link";
import AddIcon from "@mui/icons-material/AddOutlined";
import OpenIcon from "@mui/icons-material/OpenInNewOutlined";
import Typography from "@mui/material/Typography";

import { Loader } from "components/Loader/Loader";
import { OverflowY } from "components/OverflowY/OverflowY";
import { EmptyState } from "components/EmptyState/EmptyState";
import { Avatar } from "components/Avatar/Avatar";
import { SearchBox } from "./WorkspacesSearchBox";
import {
PopoverContainer,
PopoverLink,
} from "components/PopoverContainer/PopoverContainer";
import Popover from "@mui/material/Popover";

const ICON_SIZE = 18;
const COLUMN_GAP = 1.5;

function sortTemplatesByUsersDesc(
templates: readonly Template[],
searchTerm: string,
) {
const allWhitespace = /^\s+$/.test(searchTerm);
if (allWhitespace) {
return templates;
}

const termMatcher = new RegExp(searchTerm.replaceAll(/[^\w]/g, "."), "i");
return templates
.filter(
(template) =>
termMatcher.test(template.display_name) ||
termMatcher.test(template.name),
)
.sort((t1, t2) => t2.active_user_count - t1.active_user_count)
.slice(0, 10);
}

function WorkspaceResultsRow({ template }: { template: Template }) {
const theme = useTheme();

return (
<PopoverLink to={`/templates/${template.name}/workspace`}>
<Box
sx={{
display: "flex",
columnGap: COLUMN_GAP,
alignItems: "center",
paddingX: 2,
paddingY: 1,
overflowY: "hidden",
}}
>
<Avatar
src={template.icon}
fitImage
alt={template.display_name || "Coder template"}
sx={{
width: `${ICON_SIZE}px`,
height: `${ICON_SIZE}px`,
fontSize: `${ICON_SIZE * 0.5}px`,
fontWeight: 700,
}}
>
{template.display_name || "-"}
</Avatar>

<Box
sx={{
lineHeight: 1,
width: "100%",
overflow: "hidden",
color: "white",
}}
>
<Typography
component="p"
sx={{ marginY: 0, paddingBottom: 0.5, lineHeight: 1 }}
noWrap
>
{template.display_name || template.name || "[Unnamed]"}
</Typography>

<Box
component="p"
sx={{
marginY: 0,
fontSize: 14,
color: theme.palette.text.secondary,
}}
>
{/*
* There are some templates that have -1 as their user count –
* basically functioning like a null value in JS. Can safely just
* treat them as if they were 0.
*/}
{template.active_user_count <= 0
? "No"
: template.active_user_count}{" "}
developer
{template.active_user_count === 1 ? "" : "s"}
</Box>
</Box>
</Box>
</PopoverLink>
);
}

type TemplatesQuery = UseQueryResult<Template[]>;

type WorkspacesButtonProps = PropsWithChildren<{
Expand All @@ -128,13 +37,14 @@ export function WorkspacesButton({
templatesFetchStatus,
templates,
}: WorkspacesButtonProps) {
const theme = useTheme();

// Dataset should always be small enough that client-side filtering should be
// good enough. Can swap out down the line if it becomes an issue
const [searchTerm, setSearchTerm] = useState("");
const processed = sortTemplatesByUsersDesc(templates ?? [], searchTerm);

const anchorRef = useRef<HTMLButtonElement>(null);
Copy link
Member

Choose a reason for hiding this comment

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

So, I feel like I did a bad job of making my case for why I made PopoverContainer. My main goal was to abstract over what I think is a bad API from MUI – I think it encourages a lot of patterns that break React's rules, and in the future, run the risk of breaking our entire app.

Here's the link to the previous PR, where I mentioned some of the problems, but here's also a quote from the React docs page for useRef:

Do not write or read ref.current during rendering, except for initialization. This makes your component’s behavior unpredictable.

My worry is that the React team is pushing for an approach of "we reserve the right to change how things work under the hood at any time, and the only way to not get bitten is by following our rules 100%". We've already seen this with React.StrictMode breaking other companies' apps, since many weren't built to be double-called in dev mode. We're going to be getting an update in the next few days on the new React Forget compiler – there's a chance that might also "punish" more patterns that we're using right now. The React team also hasn't been afraid to break MUI itself – it still can't be used in React Server Components.

Plus, I know you've said that you've used Radix before. I'm sure you can appreciate how much simpler its Popover component is:

import * as Popover from '@radix-ui/react-popover';

export default () => (
  <Popover.Root>
    <Popover.Trigger />
    <Popover.Anchor />
    <Popover.Portal>
      <Popover.Content>
        <Popover.Close />
        <Popover.Arrow />
      </Popover.Content>
    </Popover.Portal>
  </Popover.Root>
);

Unless you have a really powerful use case, there's no need for hooks or other state to make the component function – everything just works via composition.

Copy link
Member

Choose a reason for hiding this comment

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

I think I had two goals with the component:

  1. Massage MUI's API to work strictly through composition. I'm of the mind that you can't get rid of complexity – you can only shift it around. So I'd rather something be complicated internally, than make things complicated for anything consuming it
  2. Write things in a way that insulates our codebase from these potential breaking changes in the future

To be clear, your styling changes are definitely better, and the horizontal spacing is working more consistently. And the disablePortal property helps remove the need for these dedicated links. I'm just worried that MUI's Popover could become an albatross for us down the line

Copy link
Member

Choose a reason for hiding this comment

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

But you also know more way more about the codebase than I do, so I'm going to trust whatever you think is best.

I'd be okay with accepting this PR right now, and then possibly revisiting PopoverContainer to redesign it, and get the edge cases/design issues ironed out

const [isOpen, setIsOpen] = useState(false);

let emptyState: ReactNode = undefined;
if (templates?.length === 0) {
emptyState = (
Expand All @@ -152,75 +62,186 @@ export function WorkspacesButton({
}

return (
<PopoverContainer
// Stopgap value until bug where string-based horizontal origin isn't
// being applied consistently can get figured out
originX={-115}
originY="bottom"
sx={{ display: "flex", flexFlow: "column nowrap" }}
anchorButton={
<Button startIcon={<AddIcon />} variant="contained">
{children}
</Button>
}
>
<SearchBox
value={searchTerm}
onValueChange={(newValue) => setSearchTerm(newValue)}
placeholder="Type/select a workspace template"
label="Template select for workspace"
sx={{ flexShrink: 0, columnGap: COLUMN_GAP }}
/>

<OverflowY
maxHeight={380}
sx={{
display: "flex",
flexFlow: "column nowrap",
paddingY: 1,
<>
<Button
startIcon={<AddIcon />}
variant="contained"
ref={anchorRef}
onClick={() => {
setIsOpen(true);
}}
>
{templatesFetchStatus === "loading" ? (
<Loader size={14} />
) : (
<>
{processed.map((template) => (
<WorkspaceResultsRow key={template.id} template={template} />
))}

{emptyState}
</>
)}
</OverflowY>

<Link
component={RouterLink}
to="/templates"
sx={{
outline: "none",
"&:focus": {
backgroundColor: theme.palette.action.focus,
},
{children}
</Button>
<Popover
disablePortal
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a much nicer solution than my forced closing approach, but I did notice this from the MUI docs:

The children is Portal to the body of the document to avoid rendering problems. You can disable this behavior with disablePortal.

I couldn't find anything about what these rendering problems are, though. Do you happen to know anything about this?

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 never had such problems but I would guess it can be related to style since the component would be rendered inside of the component scope and not outside of the #root element

open={isOpen}
onClose={() => setIsOpen(false)}
anchorEl={anchorRef.current}
anchorOrigin={{
vertical: "bottom",
horizontal: "right",
}}
transformOrigin={{
vertical: "top",
horizontal: "right",
}}
css={(theme) => ({
marginTop: theme.spacing(1),
Copy link
Member

Choose a reason for hiding this comment

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

What are your thoughts on replacing this with padding (probably from a parent element), just to avoid any risks of margin collapse in the future?

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 see, I think it makes sense.

"& .MuiPaper-root": {
width: theme.spacing(40),
},
})}
>
<Box
<SearchBox
value={searchTerm}
onValueChange={(newValue) => setSearchTerm(newValue)}
placeholder="Type/select a workspace template"
label="Template select for workspace"
sx={{ flexShrink: 0, columnGap: COLUMN_GAP }}
/>

<OverflowY
maxHeight={380}
sx={{
padding: 2,
display: "flex",
flexFlow: "row nowrap",
alignItems: "center",
columnGap: COLUMN_GAP,
borderTop: `1px solid ${theme.palette.divider}`,
flexDirection: "column",
paddingY: 1,
}}
>
<Box component="span" sx={{ width: `${ICON_SIZE}px` }}>
<OpenIcon
sx={{ fontSize: "16px", marginX: "auto", display: "block" }}
/>
</Box>
<span>{Language.seeAllTemplates}</span>
{templatesFetchStatus === "loading" ? (
<Loader size={14} />
) : (
<>
{processed.map((template) => (
<WorkspaceResultsRow key={template.id} template={template} />
))}

{emptyState}
</>
)}
</OverflowY>

<Box
css={(theme) => ({
padding: theme.spacing(1, 0),
borderTop: `1px solid ${theme.palette.divider}`,
})}
>
<PopoverLink
to="/templates"
css={(theme) => ({
display: "flex",
alignItems: "center",
columnGap: theme.spacing(COLUMN_GAP),

color: theme.palette.primary.main,
})}
>
<OpenIcon css={{ width: 14, height: 14 }} />
<span>See all templates</span>
</PopoverLink>
</Box>
</Link>
</PopoverContainer>
</Popover>
</>
);
}

function WorkspaceResultsRow({ template }: { template: Template }) {
return (
<PopoverLink
to={`/templates/${template.name}/workspace`}
css={(theme) => ({
display: "flex",
gap: theme.spacing(COLUMN_GAP),
alignItems: "center",
})}
>
<Avatar
src={template.icon}
fitImage
alt={template.display_name || "Coder template"}
sx={{
width: `${ICON_SIZE}px`,
height: `${ICON_SIZE}px`,
fontSize: `${ICON_SIZE * 0.5}px`,
fontWeight: 700,
}}
>
{template.display_name || "-"}
</Avatar>

<Box
css={(theme) => ({
color: theme.palette.text.primary,
display: "flex",
flexDirection: "column",
lineHeight: "140%",
fontSize: 14,
overflow: "hidden",
})}
>
<span css={{ whiteSpace: "nowrap", textOverflow: "ellipsis" }}>
{template.display_name || template.name || "[Unnamed]"}
</span>
<span
css={(theme) => ({
fontSize: 13,
color: theme.palette.text.secondary,
})}
>
{/*
* There are some templates that have -1 as their user count –
* basically functioning like a null value in JS. Can safely just
* treat them as if they were 0.
*/}
{template.active_user_count <= 0 ? "No" : template.active_user_count}{" "}
developer
{template.active_user_count === 1 ? "" : "s"}
</span>
</Box>
</PopoverLink>
);
}

function PopoverLink(props: RouterLinkProps) {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this component probably doesn't make sense now, since we have the disablePortal prop

I only needed this as a separate component, so I could have a new component boundary to call hooks from, and get closePopover via context

return (
<RouterLink
{...props}
css={(theme) => ({
color: theme.palette.text.primary,
padding: theme.spacing(1, 2),
fontSize: 14,
outline: "none",
textDecoration: "none",
"&:focus": {
backgroundColor: theme.palette.action.focus,
},
"&:hover": {
textDecoration: "none",
backgroundColor: theme.palette.action.hover,
},
})}
/>
);
}

function sortTemplatesByUsersDesc(
templates: readonly Template[],
searchTerm: string,
) {
const allWhitespace = /^\s+$/.test(searchTerm);
if (allWhitespace) {
return templates;
}

const termMatcher = new RegExp(searchTerm.replaceAll(/[^\w]/g, "."), "i");
return templates
.filter(
(template) =>
termMatcher.test(template.display_name) ||
termMatcher.test(template.name),
)
.sort((t1, t2) => t2.active_user_count - t1.active_user_count)
.slice(0, 10);
}