pierrejeambrun commented on code in PR #44854:
URL: https://github.com/apache/airflow/pull/44854#discussion_r1905476374
##########
airflow/ui/src/components/TaskInstanceTooltip.tsx:
##########
@@ -18,42 +18,54 @@
*/
import { Box, Text } from "@chakra-ui/react";
-import type { TaskInstanceHistoryResponse, TaskInstanceResponse } from
"openapi/requests/types.gen";
+import type {
+ TaskInstanceHistoryResponse,
+ TaskInstanceResponse,
+ GridTaskInstanceSummary,
+} from "openapi/requests/types.gen";
import Time from "src/components/Time";
import { Tooltip, type TooltipProps } from "src/components/ui";
type Props = {
- readonly taskInstance: TaskInstanceHistoryResponse | TaskInstanceResponse;
+ readonly taskInstance?: GridTaskInstanceSummary |
TaskInstanceHistoryResponse | TaskInstanceResponse;
} & Omit<TooltipProps, "content">;
-const TaskInstanceTooltip = ({ children, taskInstance }: Props) => (
- <Tooltip
- content={
- <Box>
- <Text>Run ID: {taskInstance.dag_run_id}</Text>
- <Text>
- Start Date: <Time datetime={taskInstance.start_date} />
- </Text>
- <Text>
- End Date: <Time datetime={taskInstance.end_date} />
- </Text>
- {taskInstance.try_number > 1 && <Text>Try Number:
{taskInstance.try_number}</Text>}
- <Text>Duration: {taskInstance.duration?.toFixed(2) ?? 0}s</Text>
- <Text>State: {taskInstance.state}</Text>
- </Box>
- }
- key={taskInstance.dag_run_id}
- positioning={{
- offset: {
- crossAxis: 5,
- mainAxis: 5,
- },
- placement: "bottom-start",
- }}
- showArrow
- >
- {children}
- </Tooltip>
-);
+const TaskInstanceTooltip = ({ children, positioning, taskInstance, ...rest }:
Props) =>
+ taskInstance === undefined ? (
+ children
+ ) : (
+ <Tooltip
+ {...rest}
+ content={
+ <Box>
+ {"dag_run_id" in taskInstance ? <Text>Run ID:
{taskInstance.dag_run_id}</Text> : undefined}
+ <Text>
+ Start Date: <Time datetime={taskInstance.start_date} />
+ </Text>
+ <Text>
+ End Date: <Time datetime={taskInstance.end_date} />
+ </Text>
+ {taskInstance.try_number > 1 && <Text>Try Number:
{taskInstance.try_number}</Text>}
+ {"duration" in taskInstance ? (
+ <Text>Duration: {taskInstance.duration?.toFixed(2) ?? 0}s</Text>
+ ) : undefined}
+ <Text>State: {taskInstance.state}</Text>
Review Comment:
State is already mentioned in the graph in plain text but also with status
color, which I think is better. I'm not sure repeating the information in plain
text in the tooltip brings a lot.
##########
airflow/ui/src/layouts/Details/Graph/Graph.tsx:
##########
@@ -52,6 +88,41 @@ export const Graph = () => {
openGroupIds,
});
+ const { data: gridData } = useGridServiceGridData(
+ {
+ dagId,
+ limit: 14,
+ offset: 0,
+ orderBy: "-start_date",
+ },
+ undefined,
+ {
+ enabled: Boolean(runId),
+ },
+ );
+
+ const dagRun = gridData?.dag_runs.find((dr) => dr.dag_run_id === runId);
+
+ console.log(runId);
+ console.log(gridData?.dag_runs);
Review Comment:
console log to remove
##########
airflow/ui/src/components/TaskInstanceTooltip.tsx:
##########
@@ -18,42 +18,54 @@
*/
import { Box, Text } from "@chakra-ui/react";
-import type { TaskInstanceHistoryResponse, TaskInstanceResponse } from
"openapi/requests/types.gen";
+import type {
+ TaskInstanceHistoryResponse,
+ TaskInstanceResponse,
+ GridTaskInstanceSummary,
+} from "openapi/requests/types.gen";
import Time from "src/components/Time";
import { Tooltip, type TooltipProps } from "src/components/ui";
type Props = {
- readonly taskInstance: TaskInstanceHistoryResponse | TaskInstanceResponse;
+ readonly taskInstance?: GridTaskInstanceSummary |
TaskInstanceHistoryResponse | TaskInstanceResponse;
} & Omit<TooltipProps, "content">;
-const TaskInstanceTooltip = ({ children, taskInstance }: Props) => (
- <Tooltip
- content={
- <Box>
- <Text>Run ID: {taskInstance.dag_run_id}</Text>
- <Text>
- Start Date: <Time datetime={taskInstance.start_date} />
- </Text>
- <Text>
- End Date: <Time datetime={taskInstance.end_date} />
- </Text>
- {taskInstance.try_number > 1 && <Text>Try Number:
{taskInstance.try_number}</Text>}
- <Text>Duration: {taskInstance.duration?.toFixed(2) ?? 0}s</Text>
- <Text>State: {taskInstance.state}</Text>
- </Box>
- }
- key={taskInstance.dag_run_id}
- positioning={{
- offset: {
- crossAxis: 5,
- mainAxis: 5,
- },
- placement: "bottom-start",
- }}
- showArrow
- >
- {children}
- </Tooltip>
-);
+const TaskInstanceTooltip = ({ children, positioning, taskInstance, ...rest }:
Props) =>
+ taskInstance === undefined ? (
+ children
+ ) : (
+ <Tooltip
+ {...rest}
+ content={
+ <Box>
+ {"dag_run_id" in taskInstance ? <Text>Run ID:
{taskInstance.dag_run_id}</Text> : undefined}
+ <Text>
+ Start Date: <Time datetime={taskInstance.start_date} />
+ </Text>
+ <Text>
+ End Date: <Time datetime={taskInstance.end_date} />
+ </Text>
+ {taskInstance.try_number > 1 && <Text>Try Number:
{taskInstance.try_number}</Text>}
+ {"duration" in taskInstance ? (
+ <Text>Duration: {taskInstance.duration?.toFixed(2) ?? 0}s</Text>
+ ) : undefined}
+ <Text>State: {taskInstance.state}</Text>
+ </Box>
+ }
+ key={taskInstance.task_id}
+ portalled
+ positioning={{
+ offset: {
+ crossAxis: 5,
+ mainAxis: 5,
+ },
+ placement: "bottom-start",
+ ...positioning,
+ }}
+ showArrow
Review Comment:
I think `showArrow` is nice. This makes me think that we should consolidate
how tooltip look in the UI, all of them with arrow or none. (I'm for all with
arrow).
Maybe reworking a little bit our tooltip component to force the arrow in all
cases might be nice.
Otherwise it feels weird to have two type of tooltip across the application.
It just feel like they are coming from two different design system.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
