pierrejeambrun commented on code in PR #62195:
URL: https://github.com/apache/airflow/pull/62195#discussion_r2854594174
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/Bar.tsx:
##########
@@ -41,10 +47,28 @@ export const Bar = ({ max, onClick, run }: Props) => {
const isSelected = runId === run.run_id;
const isHovered = hoveredRunId === run.run_id;
const search = searchParams.toString();
+ const isFailed = (run.state ?? "").toLowerCase() === "failed";
Review Comment:
Why that 'isFailed' based on the 'run.state' ? We do not set an icon for
each possible dag run states, what is the motivation here?
##########
airflow-core/src/airflow/ui/src/assets/FailedIcon.tsx:
##########
@@ -0,0 +1,41 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { createIcon } from "@chakra-ui/react";
+
+/**
+ * Warning/error icon (triangle with exclamation) for failed state.
+ * Use in UI components; for Canvas (e.g. Chart.js) use canvas drawing or an
image.
+ */
+export const FailedIcon = createIcon({
+ defaultProps: {
+ height: "1em",
+ width: "1em",
+ },
+ displayName: "Failed Icon",
Review Comment:
Same here
##########
airflow-core/src/airflow/ui/src/components/DurationChart/failedIconPlugin.ts:
##########
@@ -0,0 +1,74 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import type { Chart } from "chart.js";
+
+const FAILED_ICON_PLUGIN_ID = "durationChartFailedIcon";
+const ICON_SIZE = 14;
+const ICON_OFFSET = 4;
+
+export const createFailedIconPlugin = (failedIndices: Array<number>,
failedIconColor: string) => ({
Review Comment:
Where is that used?
##########
airflow-core/src/airflow/ui/src/assets/DeadlineIcon.tsx:
##########
@@ -0,0 +1,42 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { createIcon } from "@chakra-ui/react";
+
+/**
+ * Clock with warning indicator icon for missed deadlines.
+ * Visually distinct from the triangle FailedIcon.
+ */
+export const DeadlineIcon = createIcon({
+ defaultProps: {
+ height: "1em",
+ width: "1em",
+ },
Review Comment:
We should favor icons from react-icons library. I think there are a lot of
options and we probably don't need additional custom ones.
(unless we really can't find anything we like there)
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/Bar.tsx:
##########
@@ -41,10 +47,28 @@ export const Bar = ({ max, onClick, run }: Props) => {
const isSelected = runId === run.run_id;
const isHovered = hoveredRunId === run.run_id;
const search = searchParams.toString();
+ const isFailed = (run.state ?? "").toLowerCase() === "failed";
+ const hasMissedDeadline = Boolean(run.has_missed_deadline);
+ const barHeightPx = max > 0 ? (run.duration / max) * BAR_HEIGHT : 0;
const handleMouseEnter = () => setHoveredRunId(run.run_id);
const handleMouseLeave = () => setHoveredRunId(undefined);
+ const navigate = useNavigate();
+
+ const handleFailedIconClick = () => {
+ void navigate({ pathname: `/dags/${dagId}/runs/${run.run_id}`, search });
+ };
Review Comment:
Probably to remove.
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/Bar.tsx:
##########
@@ -41,10 +47,28 @@ export const Bar = ({ max, onClick, run }: Props) => {
const isSelected = runId === run.run_id;
const isHovered = hoveredRunId === run.run_id;
const search = searchParams.toString();
+ const isFailed = (run.state ?? "").toLowerCase() === "failed";
+ const hasMissedDeadline = Boolean(run.has_missed_deadline);
+ const barHeightPx = max > 0 ? (run.duration / max) * BAR_HEIGHT : 0;
const handleMouseEnter = () => setHoveredRunId(run.run_id);
const handleMouseLeave = () => setHoveredRunId(undefined);
+ const navigate = useNavigate();
+
+ const handleFailedIconClick = () => {
+ void navigate({ pathname: `/dags/${dagId}/runs/${run.run_id}`, search });
+ };
+
+ const handleDeadlineIconClick = () => {
+ void navigate({ pathname: `/dags/${dagId}/runs/${run.run_id}/deadlines`,
search });
+ };
+
+ // Account for minHeight and padding-bottom so icons always appear above the
rendered bar
+ const effectiveBarHeightPx = Math.max(barHeightPx, BAR_MIN_HEIGHT_PX) +
BAR_PADDING_BOTTOM_PX;
+ const failedIconBottom = effectiveBarHeightPx + ICON_GAP_PX;
+ const deadlineIconBottom = isFailed ? failedIconBottom + ICON_HEIGHT_PX :
failedIconBottom;
+
Review Comment:
This could break alignment with the 'Gantt', can you show a picture with
deadline alerts failed and the Gantt chart displayed.
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/constants.ts:
##########
@@ -22,11 +22,17 @@ export const ROW_HEIGHT = 20;
export const GRID_OUTER_PADDING_PX = 64; // pt={16} = 16 * 4 = 64px
export const GRID_HEADER_PADDING_PX = 8; // pt={2} = 2 * 4 = 8px
export const GRID_HEADER_HEIGHT_PX = 100; // height="100px" for duration bars
+// Space above bars for failed-run icon so it is not clipped
+export const GRID_HEADER_ICON_SPACE_PX = 28;
Review Comment:
This is making computation complex and easy to mess up in the Grid
component.
We should probably refactor to make it more straight forward.
--
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]
