You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "guan404ming (via GitHub)" <gi...@apache.org> on 2025/09/23 14:42:07 UTC
[PR] Replace Chart.js default tooltip with a custom Chakra UI tooltip [airflow]
guan404ming opened a new pull request, #55994:
URL: https://github.com/apache/airflow/pull/55994
<!--
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.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
## Related
#51667
## Why
- Chart.js tooltips don't match the application's design system
- Default tooltips can't adapt to light/dark mode properly
- Chart.js tooltips get squeezed/truncated when there's insufficient height, while custom tooltips maintain full
content visibility
## How
- Created GanttTooltip.tsx using Chakra UI components
- Disabled Chart.js default tooltip, positioned custom tooltip with React state
https://github.com/user-attachments/assets/cf160bbd-ce9d-44fb-9a34-70711a191d43
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments).
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace Chart.js default tooltip with a custom Chakra UI tooltip [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on PR #55994:
URL: https://github.com/apache/airflow/pull/55994#issuecomment-3324545691
I haven't looked too deeply, but the hover interaction feels slower. I wonder if the state change is forcing extra rendering
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace Chart.js default tooltip with a custom Chakra UI tooltip [airflow]
Posted by "guan404ming (via GitHub)" <gi...@apache.org>.
guan404ming commented on code in PR #55994:
URL: https://github.com/apache/airflow/pull/55994#discussion_r2372783010
##########
airflow-core/src/airflow/ui/src/layouts/Details/Gantt/Gantt.tsx:
##########
@@ -306,6 +342,15 @@ export const Gantt = ({ dagRunState, limit, runType, triggeringUser }: Props) =>
paddingTop: flatNodes.length === 1 ? 15 : 1.5,
}}
/>
+ <GanttTooltip
+ data={data}
+ selectedTimezone={selectedTimezone}
+ taskId={tooltipData.taskId}
+ translate={translate}
Review Comment:
I try to make it more efficient without initializing it again. But it would be more straightforward to call it inside the component.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace Chart.js default tooltip with a custom Chakra UI tooltip [airflow]
Posted by "guan404ming (via GitHub)" <gi...@apache.org>.
guan404ming commented on code in PR #55994:
URL: https://github.com/apache/airflow/pull/55994#discussion_r2372779095
##########
airflow-core/src/airflow/ui/src/layouts/Details/Gantt/Gantt.tsx:
##########
@@ -245,20 +260,41 @@ export const Gantt = ({ dagRunState, limit, runType, triggeringUser }: Props) =>
[data, setHoveredTaskId],
);
+ const handleCustomBarHover = useMemo(
+ () => (event: ChartEvent, elements: Array<ActiveElement>) => {
+ handleBarHover(event, elements);
+
+ if (elements.length > 0 && elements[0] && event.native) {
+ const hoveredData = data[elements[0].index];
+
+ if (hoveredData?.taskId !== undefined) {
+ setTooltipData({
+ taskId: hoveredData.taskId,
+ visible: true,
Review Comment:
it seems redundant. I've removed it.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace Chart.js default tooltip with a custom Chakra UI tooltip [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #55994:
URL: https://github.com/apache/airflow/pull/55994#discussion_r2372726310
##########
airflow-core/src/airflow/ui/src/layouts/Details/Gantt/GanttTooltip.tsx:
##########
@@ -0,0 +1,108 @@
+/*!
+ * 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, Text, VStack, useToken } from "@chakra-ui/react";
+import type { TFunction } from "i18next";
+
+import { useColorMode } from "src/context/colorMode";
+import { getDuration } from "src/utils";
+import { formatDate } from "src/utils/datetimeUtils";
+
+import type { GanttDataItem } from "./utils";
+
+type GanttTooltipProps = {
+ readonly data: Array<GanttDataItem>;
+ readonly selectedTimezone: string;
+ readonly taskId?: string;
+ readonly translate: TFunction;
+ readonly visible: boolean;
+ readonly x: number;
+ readonly y: number;
+};
+
+export const GanttTooltip = ({
+ data,
+ selectedTimezone,
+ taskId,
+ translate,
+ visible,
+ x,
+ y,
+}: GanttTooltipProps) => {
+ const { colorMode } = useColorMode();
+ const [lightBg, darkBg, lightBorder, darkBorder, lightText, darkText] = useToken("colors", [
+ "white",
+ "gray.800",
+ "gray.200",
+ "gray.600",
+ "gray.800",
+ "white",
+ ]);
Review Comment:
Colors don't match our existing tooltips either.
<img width="640" height="378" alt="Screenshot 2025-09-23 at 11 30 37 AM" src="https://github.com/user-attachments/assets/1035ad56-4473-4fc9-94ab-098d9b5f2a7b" />
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace Chart.js default tooltip with a custom Chakra UI tooltip [airflow]
Posted by "guan404ming (via GitHub)" <gi...@apache.org>.
guan404ming commented on PR #55994:
URL: https://github.com/apache/airflow/pull/55994#issuecomment-3324672105
> I am starting to feel like relying on chartjs for the gantt chart is causing too many headaches. I wonder if it would be better to just rip it all out.
I agree, I also think it might be better to slowly move away from Chart.js. We can take it step by step, maybe target 3.2 or 3.1.x.
> I haven't looked too deeply, but the hover interaction feels slower. I wonder if the state change is forcing extra rendering
yes, the hover feels noticeably slower. Probably because the state change is triggering extra re-rendering like you said. This part is kind of tricky since we don’t have direct control over the bar element (it is also a headache part), so it’ll take some testing and improvement to smooth things out.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace Chart.js default tooltip with a custom Chakra UI tooltip [airflow]
Posted by "guan404ming (via GitHub)" <gi...@apache.org>.
guan404ming closed pull request #55994: Replace Chart.js default tooltip with a custom Chakra UI tooltip
URL: https://github.com/apache/airflow/pull/55994
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace Chart.js default tooltip with a custom Chakra UI tooltip [airflow]
Posted by "guan404ming (via GitHub)" <gi...@apache.org>.
guan404ming commented on PR #55994:
URL: https://github.com/apache/airflow/pull/55994#issuecomment-3362017308
Close this one for now since rewrite the chart seems like a better option for this issue but could re-open if needed in the future.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace Chart.js default tooltip with a custom Chakra UI tooltip [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #55994:
URL: https://github.com/apache/airflow/pull/55994#discussion_r2372700594
##########
airflow-core/src/airflow/ui/src/layouts/Details/Gantt/Gantt.tsx:
##########
@@ -245,20 +260,41 @@ export const Gantt = ({ dagRunState, limit, runType, triggeringUser }: Props) =>
[data, setHoveredTaskId],
);
+ const handleCustomBarHover = useMemo(
+ () => (event: ChartEvent, elements: Array<ActiveElement>) => {
+ handleBarHover(event, elements);
+
+ if (elements.length > 0 && elements[0] && event.native) {
+ const hoveredData = data[elements[0].index];
+
+ if (hoveredData?.taskId !== undefined) {
+ setTooltipData({
+ taskId: hoveredData.taskId,
+ visible: true,
Review Comment:
Do we need `visible` or can we just check that taskId, x, y exist?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace Chart.js default tooltip with a custom Chakra UI tooltip [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #55994:
URL: https://github.com/apache/airflow/pull/55994#discussion_r2372710953
##########
airflow-core/src/airflow/ui/src/layouts/Details/Gantt/GanttTooltip.tsx:
##########
@@ -0,0 +1,108 @@
+/*!
+ * 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, Text, VStack, useToken } from "@chakra-ui/react";
+import type { TFunction } from "i18next";
+
+import { useColorMode } from "src/context/colorMode";
+import { getDuration } from "src/utils";
+import { formatDate } from "src/utils/datetimeUtils";
+
+import type { GanttDataItem } from "./utils";
+
+type GanttTooltipProps = {
+ readonly data: Array<GanttDataItem>;
+ readonly selectedTimezone: string;
+ readonly taskId?: string;
+ readonly translate: TFunction;
+ readonly visible: boolean;
+ readonly x: number;
+ readonly y: number;
+};
+
+export const GanttTooltip = ({
+ data,
+ selectedTimezone,
+ taskId,
+ translate,
+ visible,
+ x,
+ y,
+}: GanttTooltipProps) => {
+ const { colorMode } = useColorMode();
+ const [lightBg, darkBg, lightBorder, darkBorder, lightText, darkText] = useToken("colors", [
+ "white",
+ "gray.800",
+ "gray.200",
+ "gray.600",
+ "gray.800",
+ "white",
+ ]);
Review Comment:
Let's use the semantic token colors directly in `<Box />`. We only need `useToken` when we need to pass the actual hex code to a non chakra component.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace Chart.js default tooltip with a custom Chakra UI tooltip [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #55994:
URL: https://github.com/apache/airflow/pull/55994#discussion_r2372704738
##########
airflow-core/src/airflow/ui/src/layouts/Details/Gantt/Gantt.tsx:
##########
@@ -306,6 +342,15 @@ export const Gantt = ({ dagRunState, limit, runType, triggeringUser }: Props) =>
paddingTop: flatNodes.length === 1 ? 15 : 1.5,
}}
/>
+ <GanttTooltip
+ data={data}
+ selectedTimezone={selectedTimezone}
+ taskId={tooltipData.taskId}
+ translate={translate}
Review Comment:
Why are we passing translate instead of calling `useTranslate` inside of GanttTooltip?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace Chart.js default tooltip with a custom Chakra UI tooltip [airflow]
Posted by "guan404ming (via GitHub)" <gi...@apache.org>.
guan404ming commented on code in PR #55994:
URL: https://github.com/apache/airflow/pull/55994#discussion_r2372783010
##########
airflow-core/src/airflow/ui/src/layouts/Details/Gantt/Gantt.tsx:
##########
@@ -306,6 +342,15 @@ export const Gantt = ({ dagRunState, limit, runType, triggeringUser }: Props) =>
paddingTop: flatNodes.length === 1 ? 15 : 1.5,
}}
/>
+ <GanttTooltip
+ data={data}
+ selectedTimezone={selectedTimezone}
+ taskId={tooltipData.taskId}
+ translate={translate}
Review Comment:
I try to make it more efficient without initializing it again. But it would be more straightforward to call it inside the component. I've updated it.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace Chart.js default tooltip with a custom Chakra UI tooltip [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on PR #55994:
URL: https://github.com/apache/airflow/pull/55994#issuecomment-3324489553
I am starting to feel like relying on chartjs for the gantt chart is causing too many headaches. I wonder if it would be better to just rip it all out.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace Chart.js default tooltip with a custom Chakra UI tooltip [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on PR #55994:
URL: https://github.com/apache/airflow/pull/55994#issuecomment-3324785163
Let's save this change for when we try to remove chartjs from the gantt chart entirely instead of trying to hack something together.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Replace Chart.js default tooltip with a custom Chakra UI tooltip [airflow]
Posted by "guan404ming (via GitHub)" <gi...@apache.org>.
guan404ming commented on code in PR #55994:
URL: https://github.com/apache/airflow/pull/55994#discussion_r2372785644
##########
airflow-core/src/airflow/ui/src/layouts/Details/Gantt/GanttTooltip.tsx:
##########
@@ -0,0 +1,108 @@
+/*!
+ * 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, Text, VStack, useToken } from "@chakra-ui/react";
+import type { TFunction } from "i18next";
+
+import { useColorMode } from "src/context/colorMode";
+import { getDuration } from "src/utils";
+import { formatDate } from "src/utils/datetimeUtils";
+
+import type { GanttDataItem } from "./utils";
+
+type GanttTooltipProps = {
+ readonly data: Array<GanttDataItem>;
+ readonly selectedTimezone: string;
+ readonly taskId?: string;
+ readonly translate: TFunction;
+ readonly visible: boolean;
+ readonly x: number;
+ readonly y: number;
+};
+
+export const GanttTooltip = ({
+ data,
+ selectedTimezone,
+ taskId,
+ translate,
+ visible,
+ x,
+ y,
+}: GanttTooltipProps) => {
+ const { colorMode } = useColorMode();
+ const [lightBg, darkBg, lightBorder, darkBorder, lightText, darkText] = useToken("colors", [
+ "white",
+ "gray.800",
+ "gray.200",
+ "gray.600",
+ "gray.800",
+ "white",
+ ]);
Review Comment:
Thanks for catching this. I've reversed the style between dark and light.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org