Feature: Enhance Pie Chart - Enable donut chart, Set legend position, and highlight slice#7760
Feature: Enhance Pie Chart - Enable donut chart, Set legend position, and highlight slice#7760ngdaniels wants to merge 12 commits into
Conversation
🦋 Changeset detectedLatest commit: d82a544 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 |
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
@mermaid-js/examples
mermaid
@mermaid-js/layout-elk
@mermaid-js/layout-tidy-tree
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7760 +/- ##
==========================================
- Coverage 3.27% 3.26% -0.01%
==========================================
Files 600 599 -1
Lines 60657 60826 +169
Branches 916 917 +1
==========================================
+ Hits 1985 1986 +1
- Misses 58672 58840 +168
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 ↗︎
|
|
Hi @ngdaniels, [sisyphos-bot] What's Working Well 🎉 [praise] The CSS class approach for highlightSlice (pieCircleHighlighted, pieCircleHighlightedOnHover) is clean — class-based toggling is the right way to do this rather 🎉 [praise] The donutHole validation in the renderer is solid: donutHole > 0 && donutHole <= 0.9 ? donutHole : 0 silently falls back to a standard pie for out-of-range 🎉 [praise] Grouping the pie chart visuals (circle, slices, text) into a pie group element and then translating the whole group for top/left legend positions is a much Items to Address 🔴 [blocking] config.type.ts was manually edited — regenerate from schema instead config.type.ts is an auto-generated file. Per project conventions, it must only be changed by running pnpm run --filter mermaid types:build-config. The CI lint job validates The manual edits happen to produce content that matches the generator output (which is why lint passes), but the process is wrong. Please regenerate the file: pnpm run --filter mermaid types:build-config Then commit the result. Do not edit config.type.ts by hand. 🔴 [blocking] Missing changeset The PR adds three new user-facing config options (donutHole, legendPosition, highlightSlice) with no changeset file. This is required for any user-facing change: (The checklist item in your PR description is unchecked — easy to miss!) 🔴 [blocking] Default legend position changed — causes visual regression in all existing pie charts This is the source of the 14 changed Argos screenshots. The old code positioned the legend at a fixed horizontal = 12 * LEGEND_RECT_SIZE = 216px from center. The new default (right) case uses horizontal = radius + For a typical SVG height of 450px, radius = (450/2) - 40 = 185, so the new offset is 207 — close but not equal. For smaller diagrams (height=360), the old offset was 216 The fix: for the default (right) case, preserve the original formula or ensure the new formula produces the same visual for the default configuration. The safest path is to case 'right': (Or if you believe the new radius-relative positioning is better, that's a separate design discussion — but it needs Argos baseline approval from maintainers and 🟡 [important] legendPosition typed as string — should be a union type config.type.ts and config.schema.yaml both declare legendPosition?: string. The renderer only handles center, top, bottom, left, and falls through to right for anything The schema should constrain valid values: legendPosition: And config.type.ts (after regeneration) would become: This enables IDE autocomplete, catches typos at the config level, and documents the valid values explicitly. 🟡 [important] Schema maximum: 1 contradicts code cap of 0.9 config.schema.yaml declares: But pieRenderer.ts silently clamps at 0.9: Users who read the schema or docs (which say "0 to 0.9") will be confused by the mismatch. Update the schema to match the code: donutHole: 🟢 [nit] Two Cypress tests have identical descriptions pie.spec.ts (patch 7/8): Same string, different inputs — the second will shadow the first in test reporters. Rename them: 🟢 [nit] !important in styles can likely be avoided pieStyles.ts: !important overrides the .pieCircle { opacity: ... } rule. A simpler fix is higher specificity: .pieCircle.pieCircleHighlighted { ... }. This avoids the !important arms race Security No XSS or injection issues identified. highlightSlice is a config-level setting (not from diagram syntax), and label matching uses strict equality — no DOM interpolation. Self-Check
Verdict: REQUEST_CHANGES Three blocking items need attention: regenerate config.type.ts via the provided script (the manual edit is correct content but wrong process), add a changeset, and The core feature set is solid — once these are sorted this should be in good shape to land! |
|
Hi @pbrolin47 , Thanks for the feedback, really appreciate it. I've tried running |
Yes @ngdaniels , you can manually edit your changeset file to match the correct level, |
📑 Summary
This PR adds features that enhances pie chart.
Resolves #7607
Resolves #
📏 Design Decisions
Enables donut chart
Add
innerHoleproperty onpieConfig. The value is the ratio of the hole to the outer diameter. Default to 0.Set legend position
Add
legendPositionproperty onpieConfig. The value is the position of the legend, relative to the chart. Default to right.Set legend position
Add
highlightSliceproperty onpieConfig. The value is the name of the label to be highlighted. Will highlight hovered slice if value is set tohover.📋 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:.