feat(timeline): implement neo look#7406
feat(timeline): implement neo look#7406omkarht wants to merge 5 commits intofeature/neo-look-implementedfrom
Conversation
🦋 Changeset detectedLatest commit: 64d735e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
Type: Enhancement
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/neo-look-implemented #7406 +/- ##
===============================================================
- Coverage 3.59% 3.58% -0.01%
===============================================================
Files 481 480 -1
Lines 49929 50060 +131
Branches 740 740
===============================================================
Hits 1793 1793
- Misses 48136 48267 +131
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
omkarht
requested review from
ashishjain0512,
darshanr0107 and
shubhamparikh2704
knsv
left a comment
knsv
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for extending neo look to timelines, @omkarht! The comprehensive test matrix across themes and looks is great to see. A few things to address before this is ready.
What's working well
🎉 [praise] Excellent E2E test coverage — testing all 6 neo/redux themes × 2 looks, plus neo look with the 4 classic themes, gives 16+ visual snapshots. This is exactly the level of thoroughness we want for a visual feature.
🎉 [praise] Clean separation between redux and classic section generation in styles.js. The genReduxSections function keeps the new styling self-contained without entangling the existing genSections path.
Things to address
🔴 [blocking] Diagram-specific logic added to shared styles.ts. The genTimelineGradient function and the type === 'timeline' check at styles.ts:152 add timeline-specific CSS generation into the shared style orchestrator. Per CLAUDE.md: "Don't add diagram-specific logic to shared rendering code. If a rendering feature is only needed by one diagram type, it belongs in that diagram's renderer."
This gradient logic should live in diagrams/timeline/styles.js instead, where the rest of the timeline styling already is. The shared styles.ts should remain diagram-agnostic.
🔴 [blocking] configApi.getConfig() called inside styles.ts. The diff adds import * as configApi from './config.js' and calls getConfig() at runtime inside getStyles(). The shared style system receives config-derived values via the options parameter — it should not fetch config directly. This breaks the design pattern where createUserStyles() in mermaidAPI.ts resolves config and passes theme variables down. If you need theme and look in the style function, they should be passed through the existing options parameter.
The same pattern appears in diagrams/timeline/styles.js — calling configApi.getConfig() inside the styles function. Check how other diagrams do it: the options parameter already contains all theme variables. Use those instead.
🟡 [important] Hardcoded colors in genReduxSections. styles.js:56-57: fill: lightgray and fill: #efefef for disabled states are hardcoded. These should use theme variables so they adapt correctly across themes. Flag as important because hardcoded colors break the theming contract.
🟡 [important] SVG filter id="drop-shadow" is not scoped. In svgDraw.js:541, the drop shadow filter uses a fixed id="drop-shadow". If multiple timeline diagrams render on the same page, they'll share this ID, and only the first filter definition wins. Use the diagram's svgId to scope it (e.g., id="${svgId}-drop-shadow").
🟡 [important] currentDrawing string parameter is fragile. svgDraw.js:496 adds a currentDrawing parameter that accepts 'drawEvents' as a magic string. This couples the draw function to its caller by name. Consider passing a boolean like isEvent or adjusting padding via a config option instead.
🟢 [nit] Typo in changeset: "additoinal redux" → "additional redux".
🟢 [nit] Title x-position formula change. timelineRenderer.ts:210: changing from box.width / 2 - LEFT_MARGIN to box.x * 2 + LEFT_MARGIN looks like it could shift the title position for the classic look too, not just neo. Was this intentional? If so, it should be guarded by a look === 'neo' check to avoid regressing classic timelines.
🟢 [nit] Inconsistent indentation in genTimelineGradient. styles.ts:168-169: mixed spaces in the CSS template string (extra leading space on some lines). Minor, but visible in the generated CSS.
Self-check
- At least one 🎉 praise item
- No duplicate comments
- Severity tally: 2 🔴 / 3 🟡 / 3 🟢 / 0 💡 / 2 🎉
- Verdict: REQUEST_CHANGES (2 🔴 blocking items)
- Tone check: collaborative and appreciative
The core visual feature looks solid — the main issues are architectural (diagram-specific code in shared files, direct config access in style functions). Once those are moved into the timeline's own styles.js, this should come together nicely!
ashishjain0512
left a comment
ashishjain0512
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Supplementary Review: feat(timeline): implement neo look
The previous review (2026-03-01) covers the main architectural issues well. No new commits since — adding a few supplementary findings not covered in the first pass.
Additional findings
🟡 [important] isDark variable shadows khroma import
packages/mermaid/src/diagrams/timeline/styles.js:5 (in PR) — The genReduxSections function declares const isDark = theme?.includes('dark') which shadows the imported isDark function from khroma at line 1. The existing genSections function uses the khroma isDark(color) function to check actual color luminance. If someone later adds color-aware logic to genReduxSections, they'd silently get the boolean instead of the function. Worth renaming to isDarkTheme to avoid confusion.
🟡 [important] Missing linked issue
The PR body says Resolves # with no issue number. It would be helpful to link to the parent issue or design spec for the neo look feature so reviewers can verify completeness.
🟢 [nit] SVG path syntax in defaultBkg
packages/mermaid/src/diagrams/timeline/svgDraw.js (in PR, defaultBkg function) — The path q${r},0 ,${r},${r} has a space before a comma. When r=0 (redux theme), this produces degenerate q0,0 ,0,0 curves — functionally correct (sharp corners) but could be cleaner. Consider using a conditional path string: r > 0 ? 'q...' : '' to make the intent explicit.
🟢 [nit] drawTasks still uses 4-arg drawNode call
packages/mermaid/src/diagrams/timeline/timelineRenderer.ts:268 — The drawNode call in drawTasks doesn't pass the 5th currentDrawing argument, so it receives undefined. This works because the text transform adjustment only triggers for 'drawEvents', but it reinforces the existing review's point about the magic string approach being fragile. If you refactor to a boolean isEvent, the default false would be self-documenting.
Security
No XSS or injection issues. User-provided text (node.descr) flows through D3's .text() method (safe text-only insertion, not .html()). CSS interpolations use theme variables from config, not user diagram text. DOMPurify sanitization pipeline is not bypassed.
Summary
The blocking items from the first review remain the priority:
- Move
genTimelineGradientfrom sharedstyles.tsintodiagrams/timeline/styles.js - Remove
configApi.getConfig()from sharedstyles.ts— passtheme/lookviaoptions - Scope the drop-shadow filter ID
The supplementary findings here (shadowed import, missing issue link) are worth addressing in the same pass.
Self-Check
- At least one 🎉 [praise] exists (in prior review)
- No duplicate comments with prior review
- Severity tally: 0 🔴 / 2 🟡 / 2 🟢 / 0 💡 / 0 🎉
- Verdict: COMMENT (supplementary to existing REQUEST_CHANGES)
- Tone check: collaborative
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
on-behalf-of: @Mermaid-Chart <hello@mermaidchart.com>
ashishjain0512
left a comment
ashishjain0512
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Follow-up Review: feat(timeline): implement neo look
Thanks for the updates, @omkarht — nice work addressing the major architectural items from the first round. The shared styles.ts is much cleaner now, and the scoped drop-shadow ID, isEvent boolean, and theme-variable fallbacks are all solid improvements.
What's working well
🎉 [praise] Great job resolving the two blocking items. Moving genTimelineGradient out of shared styles.ts and removing getConfig() from it was exactly the right call. The shared style orchestrator is diagram-agnostic again.
🎉 [praise] The svgId pass-through change in styles.ts ({ ...options, svgId }) is a clean, diagram-agnostic improvement — it makes svgId available to all diagram style functions without adding any timeline-specific logic.
🎉 [praise] The drop-shadow filter scoping in svgDraw.js is well done — using rootSvg.attr('id') to derive a unique filter ID, checking for existing defs, and gating with .empty() to avoid duplicates. This handles the multi-diagram-per-page case correctly.
Previous review items — status
| # | Severity | Item | Status |
|---|---|---|---|
| 1 | 🔴 | Diagram-specific logic in shared styles.ts |
✅ Resolved |
| 2 | 🔴 | getConfig() in shared styles.ts |
✅ Resolved |
| 3 | 🟡 | Hardcoded disabled colors | ✅ Resolved — uses tertiaryColor/clusterBorder with fallbacks |
| 4 | 🟡 | Drop shadow ID not scoped | ✅ Resolved — now ${svgId}-drop-shadow |
| 5 | 🟡 | currentDrawing magic string |
✅ Resolved — now isEvent boolean |
| 6 | 🟡 | isDark shadowing khroma import |
✅ Resolved — renamed to isDarkTheme |
| 7 | 🟢 | Title x-position unguarded | ✅ Resolved — guarded by look === 'neo' |
| 8 | 🟡 | getConfig() in diagram styles.js |
❌ Still present — see below |
| 9 | 🟡 | Missing linked issue number | ❌ PR body still says Resolves # |
| 10 | 🟢 | Typo "additoinal" in changeset | ❌ Still present |
| 11 | 🟢 | Degenerate SVG path curves when r=0 |
❌ Still present (minor) |
Remaining items to address
🟡 [important] getConfig() still called in diagrams/timeline/styles.js
styles.js:2,120: The styles function imports getConfig and calls it to read look. The established pattern (visible in the base branch's shared styles.ts) is that style functions receive everything they need via the options parameter — they don't fetch config directly. This keeps the data flow predictable and testable.
Two approaches to fix:
- Pass
lookthrough options fromcreateUserStylesinmermaidAPI.ts(cleanest — makeslookavailable to all diagram style functions). - Use CSS attribute selectors like the shared
styles.tsdoes (e.g.,[data-look="neo"] .section-N rect { ... }) — thedata-lookattribute is already being set on nodes insvgDraw.js. This avoids needinglookat all.
🟡 [important] <linearGradient> not inside <defs>
timelineRenderer.ts:239-257: The gradient is appended directly as a child of the SVG root:
const gradient = svg.append('linearGradient')Per SVG spec, paint servers (<linearGradient>, <radialGradient>, <pattern>) should be inside <defs>. The drop-shadow filter in svgDraw.js:551-562 already follows this pattern correctly — it finds or creates <defs> and appends the filter there. It would be great to do the same for the gradient. This also lets you move it before setupGraphViewbox() for cleaner ordering.
🟡 [important] Inconsistent redux theme detection
The PR uses two different mechanisms to detect redux themes:
styles.js:119:options.dropShadow?.startsWith('url(#')— infers redux from a side effectsvgDraw.js:495:theme?.includes('redux')— checks the theme name directly
If a future theme adds dropShadow without being "redux", these would disagree. Worth standardizing on the explicit theme?.includes('redux') check. The theme value is already available in styles.js via options (as options.theme from themeVariables).
🟡 [important] Missing linked issue
The PR body still says Resolves # with no issue number. It would be helpful to link to the parent issue or design spec for the neo look feature so reviewers can verify completeness and maintainers can track the work.
🟢 [nit] Typo in changeset
.changeset/three-falcons-hang.md: "additoinal" → "additional"
🟢 [nit] Degenerate SVG path curves
svgDraw.js defaultBkg: When r=0 (redux), the path produces q0,-0,0,-0 curves. Functionally correct (renders as straight lines), but the intent would be clearer with a conditional: r > 0 ? \q0,-${r},${r},-${r}` : ''`.
Security
No XSS or injection issues identified. User-provided text (node.descr, title) flows through D3's .text() method (safe text-only insertion, not .html()). CSS interpolations use theme variables from config, not user diagram text. The DOMPurify sanitization pipeline is not bypassed by these changes.
Self-check
- At least one 🎉 [praise] item exists (3)
- No duplicate comments
- Severity tally: 0 🔴 / 4 🟡 / 2 🟢 / 0 💡 / 3 🎉
- Verdict: COMMENT (4 🟡, no blocking items — previous blockers resolved)
- Tone check: collaborative and appreciative
The two original blocking issues are resolved — great progress! The remaining items are all important-but-not-blocking improvements. The getConfig() in styles.js and the gradient <defs> placement are the highest-value fixes from this list. Looking forward to seeing this land!
3 participants
📋 Summary
This PR implements Neo look support for Timeline Diagrams, extending the Neo look system to timeline visualizations with modern styling, enhanced theming support, and comprehensive test coverage.
Resolves #
✏️ Design Decisions
1. Neo Look Integration for Timeline Diagrams
2. Comprehensive Theme Support
3. Testing Strategy
📋 Tasks
Make sure you
MERMAID_RELEASE_VERSIONis used for all new features.pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.