Add timeline component primitives#374
Conversation
802bd5d to
24cf367
Compare
|
/review |
|
✅ API Design Review completed successfully! API Design Review complete for PR #374 (Add timeline component primitives). Posted 3 inline review comments. Verdict: Request Changes — 2 Medium issues found: (1) TimelineAxis.levels is required but should be optional, symmetric with optional guides; (2) sub-component props types (TimelineRowProps, TimelineIntervalProps, etc.) are not exported, violating AppShell convention. 1 Low issue: Timeline.Link silently renders nothing for unresolved IDs with no dev-mode warning. The Tooltip.Content noArrow addition and all other aspects of the Timeline API are well-designed. Note: add_comment tool was not available in this workflow, so the overall verdict summary was not posted as a PR comment — only inline review comments were posted. |
There was a problem hiding this comment.
Generated by API Design Review for issue #374 · 122.9 AIC · ⌖ 9.67 AIC · ⊞ 5.8K
Comment /review to run again
victornguyen
left a comment
There was a problem hiding this comment.
This looks great! Very keen to make use of this in Workflow Viewer 🚀
I'm keen to get this merged so we can start playing with it and iterate through usage, but there were a couple of things my agent picked up that I thought were worth highlighting now:
1. Row labels need to be owned by the timeline
The demo hand-rolls the label column with
paddingTop: 28(a copy of the default axis level height) andheight: 40label cells (a copy of each row's height). That works until anything changes: add an axis level, and the labels drift 28px; a row resizes (which rows explicitly support — they're measured via ResizeObserver), and the external column has no way to know. Every trace/Gantt consumer needs labeled rows, so each one will rebuild this sync by hand.The machinery already exists: rows register their measured top/height with the viewport for the row-background layer. A gutter is the same registration rendering labels. Minimal ask:
label?: ReactNodeonTimeline.Row+ a gutter width on the viewport, renderedsticky; left: 0.
This was hard for me to conceptualise, so here's a Claude artifact that visually demos the failure modes:
https://claude.ai/code/artifact/4d8e34f9-9455-4561-ae6e-a55e60cb1316
2.
Timeline.Intervalneeds a minimum pixel widthIn an execution trace most bars are sub-second jobs inside runs spanning minutes: a 300ms job in a 10-min run is 0.05% ≈ 0.6px on the demo's own 1200px canvas — invisible, and an unusable click target. This is the common case for traces, not an edge case (Jaeger/Perfetto/DevTools all clamp).
Apps can't work around it:
styleis spread before the computedwidth, so overrides are discarded, and amin-widthviaclassNamefights the built-inmin-w-0. OnlyIntervalcan clamp correctly — and cheaply:width: max(${width}%, ${minWidth}px)via CSSmax(), ideally also feeding the clamped rect intoregisterItemso link anchors match the drawn bar (bodySize.widthis already measured).Ask:
minWidth?: numberonTimeline.Interval, keeping the start edge time-true.
|
Thanks — this was a very helpful review. On (1), I agree the demo currently duplicates layout values in a brittle way, so I should clean that up. That said, I’m hesitant to add I’d like to keep Timeline neutral here, since some consumers may want simple labels while others may want a table or richer side pane. If we later find a real need to synchronize intrinsic/measured row geometry across panes, I’d rather add a more generic layout-sync primitive than bake label semantics into the core API. On (2), I agree with the underlying concern: very short intervals still need to be visible and usable as click targets. I’m less convinced that I’d prefer to try that route first, and only revisit a min-width API if real usage in Workflow Viewer shows scale choice + overflowed content still aren’t enough. |
victornguyen
left a comment
There was a problem hiding this comment.
Agree on both. Happy to road test it as-is in Workflow Viewer. LGTM 🚀
Motivation
AppShell needs a reusable timeline primitive for time-based layouts. This PR adds the first public
Timelinecomponent and a demo page that shows two concrete uses: a task dependency view and a job execution timeline.This API is also meant to serve as a foundation for more specialized timeline-based experiences we expect to build next, including internal stack trace / execution-trace viewers and customer-facing planning UIs such as Gantt charts. Those use cases share the same core need — placing items against time, layering guides/labels/decorations, and expressing dependencies — but differ significantly in presentation and interaction. The goal of this PR is to establish that shared base now, so those specialized views can be built by composition instead of each introducing a separate timeline model.
Design Decision
Why introduce a primitive instead of adopting an existing library?
I also evaluated whether AppShell should adopt an existing free OSS timeline or Gantt library instead of introducing a primitive here, but the available options did not look like a good foundation for the core package.
kibo-ui's Gantt depends on extra libraries such asjotaiandlodashthat AppShell does not otherwise need,gantt-task-reactappears effectively unmaintained, and SVAR's Gantt Chart is high quality but both paywalls important features and is much more tightly specialized around a finished Gantt presentation than the lower-level building block I want here. Given those tradeoffs, I decided it was better to design a smallTimelineprimitive first and let more specialized experiences sit on top of it. That keeps the API more flexible and keeps implementation control inside AppShell.Why make
Timelinecomposable instead of shipping a finished chart?The motivation here is not just “show a Gantt.” The near-term use cases already span at least two different presentations — task dependency planning and execution/job traces — and the expected follow-up use cases diverge even more in layout, density, interaction, and labeling. What they actually share is a smaller set of primitives: a time range, a scrollable viewport, layered guides/labels/decorations, positioned intervals, and dependency links.
Designing
Timelineat that shared layer lets AppShell own the common geometry and composition model once, while leaving higher-level presentation decisions to specialized wrappers built on top. If the first API were a finished chart, later use cases would either have to contort themselves around chart-specific assumptions or introduce parallel timeline implementations anyway. The primitive-first design avoids that fork early.How are responsibilities split inside the primitive?
Timeline.Rootowns the shared time range,Timeline.Viewportowns scrolling, axis rendering, decoration layers, and link layering, and apps compose rows/items withTimeline.Row,Timeline.Interval, andTimeline.Link.Axis configuration is split into
guidesandlevelsso boundary lines and labels can be controlled independently. Row sizing and row background rendering live onTimeline.Row, which keeps viewport layout smaller and leaves presentation details with the app.Summary
Timelineprimitives and exported timeline types inpackages/coreTooltip.ContentnoArrowsupport