-
Notifications
You must be signed in to change notification settings - Fork 15.3k
Add Gantt chart view #51667
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add Gantt chart view #51667
Conversation
Known issue: the row alignment between grid view and gantt view would break when the Thanks in advance! |
Very cool! I'll pull this down and play with it some more. But my quick thoughts:
|
Very cool! |
b82c025
to
fb91fb7
Compare
I've fixed the HeaderCard height for Gantt page and do some rwd implementation for Screen.Recording.2025-06-14.at.4.25.38.PM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taiwanese Mandarin translation looks good to me
</Flex> | ||
<Stack direction={isGanttPage && containerWidth <= 380 ? "row" : "column"} mt={2} w="100%"> | ||
<HStack gap={1}>{actions}</HStack> | ||
<StatsDisplay |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice call on moving it to an Info tooltip on small sizes.
return ( | ||
<Box> | ||
<ActionButton | ||
actionName="Show stats" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Translation needed
actionName="Show stats" | ||
icon={<MdInfoOutline />} | ||
onClick={() => setIsOpen(true)} | ||
text="Stats" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too
export const HeaderCard = ({ actions, icon, isRefreshing, state, stats, subTitle, title }: Props) => { | ||
const containerRef = useRef<HTMLDivElement>(); | ||
const containerWidth = useContainerWidth(containerRef); | ||
const isGanttPage = useLocation().pathname.includes("/gantt"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this look when switching tabs? It might be better to stay consistent and always change the header based on containerWidth
<Dialog.Root lazyMount onOpenChange={() => setIsOpen(false)} open={isOpen} size="md"> | ||
<Dialog.Content backdrop> | ||
<Dialog.Header bg="blue.muted"> | ||
<Heading size="xl">{title ?? "Statistics"}</Heading> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Translation needed here as well
text="Stats" | ||
withText={false} | ||
/> | ||
<Dialog.Root lazyMount onOpenChange={() => setIsOpen(false)} open={isOpen} size="md"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a full dialog? I feel like a Popover is sufficient
@@ -72,7 +72,7 @@ const Instance = ({ dagId, isGroup, isMapped, runId, search, state, taskId }: Pr | |||
<Link | |||
replace | |||
to={{ | |||
pathname: `/dags/${dagId}/runs/${runId}/tasks/${isGroup ? "group/" : ""}${taskId}${isMapped ? "/mapped" : ""}`, | |||
pathname: `/dags/${dagId}/runs/${runId}/tasks/${isGroup ? "group/" : ""}${taskId}${isMapped ? "/mapped" : ""}/gantt`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's drop gantt
from here or let's make sure we persist whatever the selected tab is
{ icon: <MdOutlineTask />, label: "Task Instances", value: "" }, | ||
{ icon: <LuChartGantt />, label: "Gantt", value: "gantt" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we forgot to include translations here.
const data = validTaskInstances.map((taskInstance) => ({ | ||
state: taskInstance.state, | ||
x: [ | ||
dayjs(taskInstance.start_date).tz(selectedTimezone).format("YYYY-MM-DD HH:mm:ss.SSS"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to put the format as a const string at the top of the file or in datetimeUtils.ts? We repeat it a lot
I was thinking about the UX of this after #51764 Perhaps instead of putting the gantt in a tab on the right-hand panel, it should be an option to add onto the grid view on the left panel. Let's discuss first before you refactor everything though. |
I’m back! The only trade-off I see is that we might lose the shared scroll between the grid and the Gantt. What are your thoughts on that? |
Either, the gantt chart can be a separate view on the left panel (easier) |
I think the second idea is great and aligns better with the UX, especially since users will likely want to use both the grid and Gantt views at the same time. I’ll try implementing it first and see how it feels. My only concern is that it might get a bit cramped, so I might increase the min-width of the left pane to help with that. Thanks! |
Yeah, when the gantt is shown we can change the minWidth! |
d6f2fef
to
b7b5ac6
Compare
ca08ea4
to
380ca4f
Compare
Modification:
Screen.Recording.2025-06-20.at.5.12.21.PM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for the zh-TW translation and UI demo, but still need second eyes on the frontend code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the rework! Looks good to me.
Before merge would be good to have a review by one of the other peers as well. I would be good to merge it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue with having the Gantt next to the Grid is that I'm not sure we will have enough real eastate to cover all cases.
For instance if I start showing last 50 runs, or even last 100 runs, it's impossible to see the Gantt at the moment.
last 50 runs displayed (I could minimize the right panel to have more space)
last 100 runs
Also that leaves a big scroll. I don't know maybe a separate page at first would be much easier to handle all cases, and prevent enormous horizontal scroll. No strong opinion on this, you guys seemed all aligned on the "Gantt next to the Grid" so if you want to keep it like this it's fine
I acknowledge that it is hard to use the Gantt in case you display ~50 or 100 runs. Maybe we should disable the Gantt by default if you pick more than 10 runs. Separate page makes the usage harder I believe. And we had challenges aligning it on the right panel... but I like having it then you can directly select tasks that are long-runners and check logs or details. |
Thanks for the suggestions. They make sense to me and I would continue to polish the chart after my vacation til Saturday. I would also check the implementation is suitable or not for our current code base and would raise discussion here if there is any problems. Let me mark this as draft temporarily. |
Related Issue
#44672
cc @bbovenzi
Why
Our ui currently lack of gantt chart compared with AF2
How
Reimplement the gantt chart with chart.js
chartjs-adapter-dayjs-4
to use dayjs to format time in chart.jsRun
,TI
andGroupTI
pageScreen.Recording.2025-06-13.at.1.49.32.AM.mov
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
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.