jscheffl commented on code in PR #55400:
URL: https://github.com/apache/airflow/pull/55400#discussion_r2332094356
##########
providers/edge3/src/airflow/providers/edge3/plugins/www/src/components/MaintenanceExitButton.tsx:
##########
@@ -48,14 +51,43 @@ export const MaintenanceExitButton = ({ onExitMaintenance,
workerName }: Mainten
};
return (
- <IconButton
- size="sm"
- variant="ghost"
- onClick={() => exitMaintenance()}
- aria-label="Exit Maintenance"
- title="Exit Maintenance"
- >
- <IoMdExit />
- </IconButton>
+ <>
+ <IconButton
+ size="sm"
+ variant="ghost"
+ onClick={onOpen}
+ aria-label="Exit Maintenance"
+ title="Exit Maintenance"
+ >
+ <IoMdExit />
+ </IconButton>
+
+ <Dialog.Root onOpenChange={onClose} open={open} size="md">
+ <Portal>
+ <Dialog.Backdrop />
+ <Dialog.Positioner>
+ <Dialog.Content>
+ <Dialog.Header>
+ <Dialog.Title>Exit maintenance for worker
{workerName}</Dialog.Title>
+ </Dialog.Header>
+ <Dialog.Body>
+ <p>Are you sure you want to exit maintenance mode for worker
{workerName}?</p>
Review Comment:
I think we do not need a `<p>` wrapped....
In other dialogs in core UI it is wrapped with a `<Text>` can you switch to
this? Will be cleaner
##########
providers/edge3/src/airflow/providers/edge3/plugins/www/src/components/MaintenanceExitButton.tsx:
##########
@@ -48,14 +51,43 @@ export const MaintenanceExitButton = ({ onExitMaintenance,
workerName }: Mainten
};
return (
- <IconButton
- size="sm"
- variant="ghost"
- onClick={() => exitMaintenance()}
- aria-label="Exit Maintenance"
- title="Exit Maintenance"
- >
- <IoMdExit />
- </IconButton>
+ <>
+ <IconButton
+ size="sm"
+ variant="ghost"
+ onClick={onOpen}
+ aria-label="Exit Maintenance"
+ title="Exit Maintenance"
+ >
+ <IoMdExit />
+ </IconButton>
+
+ <Dialog.Root onOpenChange={onClose} open={open} size="md">
+ <Portal>
+ <Dialog.Backdrop />
+ <Dialog.Positioner>
+ <Dialog.Content>
+ <Dialog.Header>
+ <Dialog.Title>Exit maintenance for worker
{workerName}</Dialog.Title>
+ </Dialog.Header>
+ <Dialog.Body>
+ <p>Are you sure you want to exit maintenance mode for worker
{workerName}?</p>
+ </Dialog.Body>
+ <Dialog.Footer>
+ <Dialog.ActionTrigger asChild>
+ <Button variant="outline">No</Button>
+ </Dialog.ActionTrigger>
+ <Button onClick={exitMaintenance} colorScheme="red">
Review Comment:
For entering the maintenance we have no special color - should we change the
other to red as well for consistency?
I assume you took over the code from delete button... which is more
"dangerous". Not fully sure if we should mark it "red" as being dangerous or
make both dialogs consistent?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
