guan404ming commented on code in PR #53216: URL: https://github.com/apache/airflow/pull/53216#discussion_r2773642696
########## airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx: ########## @@ -0,0 +1,123 @@ +/*! + * 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 { Box } from "@chakra-ui/react"; +import { useTranslation } from "react-i18next"; +import { FiGitCommit } from "react-icons/fi"; + +import { Tooltip } from "src/components/ui"; + +type BundleVersionIndicatorProps = { + readonly bundleVersion: string | undefined; +}; + +export const BundleVersionIndicator = ({ bundleVersion }: BundleVersionIndicatorProps) => { + const { t: translate } = useTranslation("components"); + + return ( + <Tooltip content={`${translate("versionDetails.bundleVersion")}: ${bundleVersion}`}> + <Box color="orange.focusRing" left={-2} position="absolute" top={93} zIndex={1}> + <FiGitCommit size={15} /> + </Box> + </Tooltip> + ); +}; + +type DagVersionIndicatorProps = { + readonly dagVersionNumber: number | undefined; + readonly orientation?: "horizontal" | "vertical"; +}; + +export const DagVersionIndicator = ({ + dagVersionNumber, + orientation = "vertical", +}: DagVersionIndicatorProps) => { + const isVertical = orientation === "vertical"; + + const containerStyles = { + horizontal: { + height: 0.5, + left: "50%", + top: 0, + transform: "translate(-50%, -50%)", + width: 4.5, + }, + vertical: { + height: 104, + left: -1.25, + top: -1.5, + width: 0.5, + }, + } as const; + + const circleStyles = { Review Comment: ditto ########## airflow-core/src/airflow/ui/src/layouts/Details/PanelButtons.tsx: ########## @@ -471,6 +493,64 @@ export const PanelButtons = ({ ) : undefined} </> )} + {/* eslint-disable react/jsx-max-depth */} Review Comment: Nit: Shall we try to make this a separated components like `VersionIndicatorSelect` since this component is kind of over complex (for me)? ########## airflow-core/src/airflow/ui/src/layouts/Details/DetailsLayout.tsx: ########## @@ -79,6 +81,11 @@ export const DetailsLayout = ({ children, error, isLoading, tabs }: Props) => { ); const [showGantt, setShowGantt] = useLocalStorage<boolean>(`show_gantt-${dagId}`, false); + const [showVersionIndicatorMode, setShowVersionIndicatorMode] = Review Comment: Maybe we could use `version_indicator_display_mode-${dagId}` for consistency, or document why this is intentionally global. ########## airflow-core/src/airflow/ui/src/constants/showVersionIndicatorOptions.ts: ########## @@ -0,0 +1,48 @@ +/*! + * 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 { createListCollection } from "@chakra-ui/react"; + +export enum VersionIndicatorDisplayOptions { + ALL = "all", + BUNDLE = "bundle", + DAG = "dag", + NONE = "none", +} + +export type VersionIndicatorDisplayOption = VersionIndicatorDisplayOptions; Review Comment: I think this might be kind of redundant. Consider removing the alias and using the enum directly everywhere, or using a union type ("all" | "bundle" | "dag" | "none") to decouple from the enum. ########## airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx: ########## @@ -0,0 +1,123 @@ +/*! + * 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 { Box } from "@chakra-ui/react"; +import { useTranslation } from "react-i18next"; +import { FiGitCommit } from "react-icons/fi"; + +import { Tooltip } from "src/components/ui"; + +type BundleVersionIndicatorProps = { + readonly bundleVersion: string | undefined; +}; + +export const BundleVersionIndicator = ({ bundleVersion }: BundleVersionIndicatorProps) => { + const { t: translate } = useTranslation("components"); + + return ( + <Tooltip content={`${translate("versionDetails.bundleVersion")}: ${bundleVersion}`}> + <Box color="orange.focusRing" left={-2} position="absolute" top={93} zIndex={1}> + <FiGitCommit size={15} /> + </Box> + </Tooltip> + ); +}; + +type DagVersionIndicatorProps = { + readonly dagVersionNumber: number | undefined; + readonly orientation?: "horizontal" | "vertical"; +}; + +export const DagVersionIndicator = ({ + dagVersionNumber, + orientation = "vertical", +}: DagVersionIndicatorProps) => { + const isVertical = orientation === "vertical"; + + const containerStyles = { Review Comment: The objects are recreated on every render. Since they're static constants, we could move outside the component to avoid unnecessary object allocations. ########## airflow-core/src/airflow/api_fastapi/core_api/services/ui/grid.py: ########## @@ -69,11 +69,15 @@ def _get_aggs_for_node(detail): max_end_date = max(x["end_date"] for x in detail if x["end_date"]) except ValueError: max_end_date = None + + dag_version_number = detail[0].get("dag_version_number") Review Comment: Should we use `max()` here, which is consistent with how `dag_version_number` is computed for runs. -- 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]
