-
Notifications
You must be signed in to change notification settings - Fork 11
fix: Init target variable ui #171
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,221 @@ | ||
| import React, { useState } from "react"; | ||
| import { z } from "zod"; | ||
|
|
||
| import { Button } from "@ctrlplane/ui/button"; | ||
| import { Checkbox } from "@ctrlplane/ui/checkbox"; | ||
| import { | ||
| Dialog, | ||
| DialogContent, | ||
| DialogFooter, | ||
| DialogHeader, | ||
| DialogTrigger, | ||
| } from "@ctrlplane/ui/dialog"; | ||
| import { | ||
| Form, | ||
| FormControl, | ||
| FormField, | ||
| FormItem, | ||
| FormLabel, | ||
| FormMessage, | ||
| useForm, | ||
| } from "@ctrlplane/ui/form"; | ||
| import { Input } from "@ctrlplane/ui/input"; | ||
| import { | ||
| Select, | ||
| SelectContent, | ||
| SelectItem, | ||
| SelectTrigger, | ||
| SelectValue, | ||
| } from "@ctrlplane/ui/select"; | ||
|
|
||
| import { api } from "~/trpc/react"; | ||
|
|
||
| type CreateTargetVariableDialogProps = { | ||
| targetId: string; | ||
| existingKeys: string[]; | ||
| children: React.ReactNode; | ||
| }; | ||
|
|
||
| export const CreateTargetVariableDialog: React.FC< | ||
| CreateTargetVariableDialogProps | ||
| > = ({ targetId, existingKeys, children }) => { | ||
| const [open, setOpen] = useState(false); | ||
| const createTargetVariable = api.target.variable.create.useMutation(); | ||
| const schema = z.object({ | ||
| key: z | ||
| .string() | ||
| .refine((k) => k.length > 0, { message: "Key is required" }) | ||
| .refine((k) => !existingKeys.includes(k), { | ||
| message: "Variable key must be unique", | ||
| }), | ||
| type: z.enum(["string", "number", "boolean"]), | ||
| value: z | ||
| .union([z.string(), z.number(), z.boolean()]) | ||
| .refine((v) => (typeof v === "string" ? v.length > 0 : true), { | ||
| message: "Value is required", | ||
| }), | ||
| sensitive: z.boolean().default(false), | ||
| }); | ||
| const form = useForm({ | ||
| schema, | ||
| defaultValues: { key: "", value: "", type: "string" }, | ||
| }); | ||
| const { sensitive, type } = form.watch(); | ||
|
|
||
| const utils = api.useUtils(); | ||
| const onSubmit = form.handleSubmit((data) => | ||
| createTargetVariable | ||
| .mutateAsync({ targetId, ...data }) | ||
| .then(() => utils.target.byId.invalidate(targetId)) | ||
| .then(() => form.reset()) | ||
| .then(() => setOpen(false)), | ||
| ); | ||
|
|
||
| return ( | ||
| <Dialog open={open} onOpenChange={setOpen}> | ||
| <DialogTrigger asChild>{children}</DialogTrigger> | ||
| <DialogContent> | ||
| <DialogHeader>Create Target Variable</DialogHeader> | ||
| <Form {...form}> | ||
| <form onSubmit={onSubmit} className="space-y-4"> | ||
| <FormField | ||
| control={form.control} | ||
| name="key" | ||
| render={({ field }) => ( | ||
| <FormItem> | ||
| <FormLabel>Key</FormLabel> | ||
| <FormControl> | ||
| <Input {...field} /> | ||
| </FormControl> | ||
| <FormMessage /> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| <FormField | ||
| control={form.control} | ||
| name="type" | ||
| render={({ field: { value, onChange } }) => { | ||
| const onTypeChange = (type: string) => { | ||
| if (type === "string") form.setValue("value", ""); | ||
| if (type === "number") form.setValue("value", 0); | ||
| if (type === "boolean") form.setValue("value", false); | ||
| if (type !== "string") form.setValue("sensitive", false); | ||
| onChange(type); | ||
| }; | ||
|
|
||
| return ( | ||
| <FormItem> | ||
| <FormLabel>Type</FormLabel> | ||
| <FormControl> | ||
| <Select value={value} onValueChange={onTypeChange}> | ||
| <SelectTrigger> | ||
| <SelectValue placeholder="Variable type..." /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| <SelectItem value="string">String</SelectItem> | ||
| <SelectItem value="number">Number</SelectItem> | ||
| <SelectItem value="boolean">Boolean</SelectItem> | ||
| </SelectContent> | ||
| </Select> | ||
| </FormControl> | ||
| <FormMessage /> | ||
| </FormItem> | ||
| ); | ||
| }} | ||
| /> | ||
|
|
||
| {type === "string" && ( | ||
| <FormField | ||
| control={form.control} | ||
| name="value" | ||
| render={({ field: { value, onChange } }) => ( | ||
| <FormItem> | ||
| <FormLabel>Value</FormLabel> | ||
| <FormControl> | ||
| <Input | ||
| value={value as string} | ||
| onChange={onChange} | ||
| type={sensitive ? "password" : "text"} | ||
| /> | ||
| </FormControl> | ||
| <FormMessage /> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| )} | ||
|
|
||
| {type === "number" && ( | ||
| <FormField | ||
| control={form.control} | ||
| name="value" | ||
| render={({ field: { value, onChange } }) => ( | ||
| <FormItem> | ||
| <FormLabel>Value</FormLabel> | ||
| <FormControl> | ||
| <Input | ||
| value={value as number} | ||
| onChange={onChange} | ||
| type="number" | ||
| /> | ||
| </FormControl> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| )} | ||
|
|
||
| {type === "boolean" && ( | ||
| <FormField | ||
| control={form.control} | ||
| name="value" | ||
| render={({ field: { value, onChange } }) => ( | ||
| <FormItem> | ||
| <FormLabel>Value</FormLabel> | ||
| <FormControl> | ||
| <Select | ||
| value={value ? "true" : "false"} | ||
| onValueChange={(v) => onChange(v === "true")} | ||
| > | ||
| <SelectTrigger> | ||
| <SelectValue placeholder="Value..." /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| <SelectItem value="true">True</SelectItem> | ||
| <SelectItem value="false">False</SelectItem> | ||
| </SelectContent> | ||
| </Select> | ||
| </FormControl> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| )} | ||
|
|
||
| {type === "string" && ( | ||
| <FormField | ||
| control={form.control} | ||
| name="sensitive" | ||
| render={({ field: { value, onChange } }) => ( | ||
| <FormItem> | ||
| <FormControl> | ||
| <div className="flex items-center gap-2"> | ||
| <Checkbox checked={value} onCheckedChange={onChange} /> | ||
| <label htmlFor="sensitive" className="text-sm"> | ||
| Sensitive | ||
| </label> | ||
| </div> | ||
| </FormControl> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| )} | ||
|
|
||
| <DialogFooter> | ||
| <Button type="submit" disabled={createTargetVariable.isPending}> | ||
| Create | ||
| </Button> | ||
| </DialogFooter> | ||
| </form> | ||
| </Form> | ||
| </DialogContent> | ||
| </Dialog> | ||
| ); | ||
|
Comment on lines
+74
to
+220
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance accessibility and user experience. Consider these accessibility improvements:
-<Dialog open={open} onOpenChange={setOpen}>
+<Dialog
+ open={open}
+ onOpenChange={setOpen}
+ aria-label="Create Target Variable Dialog"
+>Also, consider adding a description for the sensitive checkbox: <div className="flex items-center gap-2">
<Checkbox checked={value} onCheckedChange={onChange} />
<label htmlFor="sensitive" className="text-sm">
Sensitive
</label>
+ <p className="text-sm text-muted-foreground">
+ Mark this variable as sensitive to mask its value in the UI
+ </p>
</div>
|
||
| }; | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,67 @@ | ||||||||||||||||||||||||||||||
| import React, { useState } from "react"; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||
| AlertDialog, | ||||||||||||||||||||||||||||||
| AlertDialogAction, | ||||||||||||||||||||||||||||||
| AlertDialogCancel, | ||||||||||||||||||||||||||||||
| AlertDialogContent, | ||||||||||||||||||||||||||||||
| AlertDialogDescription, | ||||||||||||||||||||||||||||||
| AlertDialogFooter, | ||||||||||||||||||||||||||||||
| AlertDialogHeader, | ||||||||||||||||||||||||||||||
| AlertDialogTrigger, | ||||||||||||||||||||||||||||||
| } from "@ctrlplane/ui/alert-dialog"; | ||||||||||||||||||||||||||||||
| import { buttonVariants } from "@ctrlplane/ui/button"; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| import { api } from "~/trpc/react"; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| type DeleteTargetVariableDialogProps = { | ||||||||||||||||||||||||||||||
| variableId: string; | ||||||||||||||||||||||||||||||
| targetId: string; | ||||||||||||||||||||||||||||||
| onClose: () => void; | ||||||||||||||||||||||||||||||
| children: React.ReactNode; | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| export const DeleteTargetVariableDialog: React.FC< | ||||||||||||||||||||||||||||||
| DeleteTargetVariableDialogProps | ||||||||||||||||||||||||||||||
| > = ({ variableId, targetId, onClose, children }) => { | ||||||||||||||||||||||||||||||
| const [open, setOpen] = useState(false); | ||||||||||||||||||||||||||||||
| const deleteTargetVariable = api.target.variable.delete.useMutation(); | ||||||||||||||||||||||||||||||
| const utils = api.useUtils(); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
Comment on lines
+24
to
+30
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider adding loading state management. The component could benefit from handling loading states during the deletion process to prevent multiple clicks and provide better user feedback. export const DeleteTargetVariableDialog: React.FC<
DeleteTargetVariableDialogProps
> = ({ variableId, targetId, onClose, children }) => {
const [open, setOpen] = useState(false);
+ const [isDeleting, setIsDeleting] = useState(false);
const deleteTargetVariable = api.target.variable.delete.useMutation();
const utils = api.useUtils();📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||
| const onDelete = () => | ||||||||||||||||||||||||||||||
| deleteTargetVariable | ||||||||||||||||||||||||||||||
| .mutateAsync(variableId) | ||||||||||||||||||||||||||||||
| .then(() => utils.target.byId.invalidate(targetId)) | ||||||||||||||||||||||||||||||
| .then(() => setOpen(false)); | ||||||||||||||||||||||||||||||
|
Comment on lines
+31
to
+35
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enhance error handling and user feedback in delete operation. The current implementation lacks error handling and user feedback. Consider implementing a more robust deletion flow. - const onDelete = () =>
- deleteTargetVariable
- .mutateAsync(variableId)
- .then(() => utils.target.byId.invalidate(targetId))
- .then(() => setOpen(false));
+ const onDelete = async () => {
+ try {
+ setIsDeleting(true);
+ await deleteTargetVariable.mutateAsync(variableId);
+ await utils.target.byId.invalidate(targetId);
+ setOpen(false);
+ // Consider adding a toast notification for success
+ } catch (error) {
+ // Consider adding error handling UI (e.g., toast notification)
+ console.error('Failed to delete target variable:', error);
+ } finally {
+ setIsDeleting(false);
+ }
+ };
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||
| <AlertDialog | ||||||||||||||||||||||||||||||
| open={open} | ||||||||||||||||||||||||||||||
| onOpenChange={(o) => { | ||||||||||||||||||||||||||||||
| setOpen(o); | ||||||||||||||||||||||||||||||
| if (!o) onClose(); | ||||||||||||||||||||||||||||||
| }} | ||||||||||||||||||||||||||||||
| > | ||||||||||||||||||||||||||||||
| <AlertDialogTrigger asChild>{children}</AlertDialogTrigger> | ||||||||||||||||||||||||||||||
| <AlertDialogContent> | ||||||||||||||||||||||||||||||
| <AlertDialogHeader>Are you sure?</AlertDialogHeader> | ||||||||||||||||||||||||||||||
| <AlertDialogDescription> | ||||||||||||||||||||||||||||||
| Deleting a target variable can change what values are passed to | ||||||||||||||||||||||||||||||
| pipelines running for this target. | ||||||||||||||||||||||||||||||
| </AlertDialogDescription> | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| <AlertDialogFooter> | ||||||||||||||||||||||||||||||
| <AlertDialogCancel>Cancel</AlertDialogCancel> | ||||||||||||||||||||||||||||||
| <div className="flex-grow" /> | ||||||||||||||||||||||||||||||
| <AlertDialogAction | ||||||||||||||||||||||||||||||
| className={buttonVariants({ variant: "destructive" })} | ||||||||||||||||||||||||||||||
| onClick={onDelete} | ||||||||||||||||||||||||||||||
| disabled={deleteTargetVariable.isPending} | ||||||||||||||||||||||||||||||
| > | ||||||||||||||||||||||||||||||
| Delete | ||||||||||||||||||||||||||||||
| </AlertDialogAction> | ||||||||||||||||||||||||||||||
| </AlertDialogFooter> | ||||||||||||||||||||||||||||||
| </AlertDialogContent> | ||||||||||||||||||||||||||||||
| </AlertDialog> | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Simplify boolean value selection.
Consider using a Checkbox component instead of a Select for boolean values, as it's more intuitive and accessible.
📝 Committable suggestion