Skip to content

feat: rename workspace #163

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.

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 2 commits into from
Jan 22, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 10 additions & 1 deletion src/features/workspace/components/workspace-creation.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,24 @@ import {
TextField,
} from "@stacklok/ui-kit";
import { FormEvent, useState } from "react";
import { useNavigate } from "react-router-dom";

export function WorkspaceCreation() {
const navigate = useNavigate();
const [workspaceName, setWorkspaceName] = useState("");
const { mutate, isPending, error } = useCreateWorkspace();
const errorMsg = error?.detail ? `${error?.detail}` : "";

const handleSubmit = (e: FormEvent<HTMLFormElement>) => {
e.preventDefault();
mutate({ body: { name: workspaceName } });
mutate(
{
body: { name: workspaceName },
},
{
onSuccess: () => navigate("/workspaces"),
},
);
};

return (
Expand Down
53 changes: 44 additions & 9 deletions src/features/workspace/components/workspace-name.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,16 @@
import { Card, CardBody, Input, Label, TextField } from "@stacklok/ui-kit";
import {
Button,
Card,
CardBody,
CardFooter,
Form,
Input,
Label,
TextField,
} from "@stacklok/ui-kit";
import { twMerge } from "tailwind-merge";
import { useCreateWorkspace } from "../hooks/use-create-workspace";
import { FormEvent, useState } from "react";

export function WorkspaceName({
className,
Expand All @@ -8,14 +19,38 @@ export function WorkspaceName({
className?: string;
workspaceName: string;
}) {
const [name, setName] = useState(workspaceName);
const { mutate, isPending, error } = useCreateWorkspace();
const errorMsg = error?.detail ? `${error?.detail}` : "";

const handleSubmit = (e: FormEvent<HTMLFormElement>) => {
e.preventDefault();
mutate({ body: { name: workspaceName, rename_to: name } });
};

return (
Comment on lines +26 to +30
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a hook in the system prompt editor useSavedStatus

function useSavedStatus() {
const [saved, setSaved] = useState<boolean>(false);
useEffect(() => {
const id = setTimeout(() => setSaved(false), 2000);
return () => clearTimeout(id);
}, [saved]);
return { saved, setSaved };
}

We could re-use it here to show a pending+confirmation state

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mm I saw it, honestly I don't see the value 🤔
I mean we have isPending and the onSuccess callback for notification, why we need this hook?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to provide visual feedback that it has been saved — also for consistency.
Feel free to merge without this — I would like to go back and standardise it - but it's deffo not a blocker.

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 mean, I would prefer to show a notification after a mutation, I would standardise the notification approach after a mutation...wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a valid option too

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's merge as is and standardise on something in next iteration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

makes sense 👍

<Card className={twMerge(className, "shrink-0")}>
<CardBody>
<TextField value={workspaceName} isReadOnly>
<Label>Workspace name</Label>
<Input />
</TextField>
</CardBody>
</Card>
<Form onSubmit={handleSubmit} validationBehavior="aria">
<Card className={twMerge(className, "shrink-0")}>
<CardBody>
<TextField
aria-label="Workspace name"
value={name}
name="Workspace name"
validationBehavior="aria"
isRequired
onChange={setName}
>
<Label>Workspace name</Label>
<Input />
{errorMsg && <div className="p-1 text-red-700">{errorMsg}</div>}
</TextField>
</CardBody>
<CardFooter className="justify-end gap-2">
<Button isDisabled={name === ""} isPending={isPending} type="submit">
Save
</Button>
</CardFooter>
</Card>
</Form>
);
}
3 changes: 0 additions & 3 deletions src/features/workspace/hooks/use-create-workspace.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
import { useMutation } from "@tanstack/react-query";
import { useNavigate } from "react-router-dom";
import { v1CreateWorkspaceMutation } from "@/api/generated/@tanstack/react-query.gen";

export function useCreateWorkspace() {
const navigate = useNavigate();
return useMutation({
...v1CreateWorkspaceMutation(),
onSuccess: () => navigate("/workspaces"),
});
}
2 changes: 1 addition & 1 deletion src/routes/__tests__/route-workspace.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ test("renders title", () => {
test("renders workspace name input", () => {
const { getByRole } = renderComponent();

expect(getByRole("textbox", { name: "Workspace name" })).toBeVisible();
expect(getByRole("textbox", { name: /workspace name/i })).toBeVisible();
});

test("renders system prompt editor", async () => {
Expand Down
Loading