Skip to content

docs: correct the donut shape control and document the center total - #11520

Open
igorlukanin wants to merge 2 commits into
masterfrom
igor/cub-3186-donut-total-docs
Open

docs: correct the donut shape control and document the center total#11520
igorlukanin wants to merge 2 commits into
masterfrom
igor/cub-3186-donut-total-docs

Conversation

@igorlukanin

Copy link
Copy Markdown
Member

The pie & donut page describes an Inner radius slider that no longer exists — the control is now a Pie / Donut shape selector in the Style tab, and the hole is a fixed fraction of the chart rather than a pixel value a user drags.

It also suggested overlaying a KPI tile on a dashboard to get a summary value in the hole. The donut now renders the measure's grand total itself, behind a Show total switch, so that workaround is no longer the answer.

Updates both sections to what the product actually does. No new pages; the screenshot placeholders are carried over and re-worded to match.

@igorlukanin
igorlukanin requested a review from keydunov as a code owner August 11, 2026 02:59
@mintlify

mintlify Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
cubed3 🟢 Ready View Preview Aug 11, 2026, 3:00 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 1m 16s —— View job


Review: docs accuracy fix — looks good, one trim suggested

Verdict: approve with nits. 0 high, 1 medium, 3 low. The correction itself is right and the page reads better than before. One inline comment posted.

Full review — todos, findings, and notes

Todos

  • Gather context (diff, full file, sibling chart pages, docs conventions)
  • Understand the request — PR review
  • Check for broken anchors / cross-links elsewhere in the docs
  • Check claims against source (chart UI is not in this repo — see caveat)
  • Check dedup against prior review threads (none existed)
  • Post review

What's good

  • The core correction is the valuable part: an Inner radius slider that doesn't exist is worse than no docs, and the KPI-tile-overlay workaround actively pointed users at the wrong solution. Both are gone.
  • Section rename from ## Inner radius## Show total correctly follows the semantics of the control rather than keeping a stale heading alive.
  • Screenshot placeholders were re-worded rather than dropped, so the outstanding image work doesn't get silently lost.
  • Line 26 first clause ("appears only for a donut") preempts the obvious support question — good instinct.

Findings

Medium — pie.mdx:26, state-retention sentence (inline)
"Switching back to a pie hides the total and remembers the setting, so returning to a donut restores it" documents transient UI state. It's padding per docs-mintlify/CLAUDE.md ("Do not pad… write the shortest version that fully answers 'how do I use this?'"), and it's the line most likely to go stale without anyone noticing. Suggested cut in the inline comment.

Low — pie.mdx:24, redundant clause
"The total uses the measure's own number format, so changing the format of the measure changes the total with it." The so clause restates the first clause. Consider just: "The total uses the measure's number format."

Low — pie.mdx:22, heading is a UI label
Sibling pages use conceptual headings (## Stacking, ## Color and slice ordering, ## Variants). ## Show total is a literal switch label. ## Center total with Show total bolded in the prose would match the house style better — though the literal label is arguably more findable via search, so this is a judgment call.

Low — dropped #inner-radius anchor and KPI cross-link
Nothing in-repo links to pie#inner-radius (verified by grep across .mdx and docs.json), so no internal breakage. External inbound links to that anchor will land at the top of the page instead. Not worth preserving via {#inner-radius} here since the section's meaning changed entirely — flagging only for awareness. Same for the removed link to the KPI page: correct to drop the bad workaround, just noting pie.mdx no longer cross-links to kpi.mdx at all.

Verification caveat

The factual claims — that the control is a Shape selector, that Show total exists and is donut-only, that the total inherits the measure's number format, and that the setting persists across shape switches — cannot be verified from this repo. grep for showTotal / innerRadius across all .ts/.tsx/.js returns nothing; the chart configuration UI lives in the Cube Cloud frontend. I'm taking these on the author's word and reviewing for docs quality, consistency, and link integrity only.

The other review dimensions in the standing instructions (security, performance, error handling, test coverage) don't apply — this is a 7-line prose change to a single .mdx file with no code, no build step, and no test surface.

· branch [`igor/cub-3186-donut-total-docs`](https://github.com/cube-js/cube/tree/igor/cub-3186-donut-total-docs)

Comment thread docs-mintlify/docs/explore-analyze/charts/chart-types/pie.mdx Outdated
@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 45s —— View job


Re-review after 9f6a395 — all prior nits addressed, no new findings

Verdict: approve. 0 high, 0 medium, 0 low. No new inline comments; my one prior thread is already resolved.

Full review — todos, findings, and notes

Todos

  • Re-read diff and full file at 9f6a395
  • Check prior review threads for staleness / dedup
  • Re-verify link and anchor integrity
  • Post review

What changed since the last review

9f6a395 addressed every point from the prior pass:

  • Medium (state retention) — the "remembers the setting, so returning to a donut restores it" clause is gone. Line 26 is now just The option appears only for a donut, since a pie has no hollow center to fill. The thread was resolved by the author; nothing left to re-affirm.
  • Low (redundant clause)pie.mdx:24 trimmed to "The total uses the measure's number format." The restating so-clause is gone.
  • Low (UI-label heading) — heading is now ## Center total with Show total bolded in the prose, matching the conceptual-heading style of the sibling pages (## Stacking, ## Variants).

Link and anchor integrity

Re-verified on the current tree:

  • grep for inner-radius / Inner radius / innerRadius across docs-mintlify/ returns nothing — no stale references to the removed control or its anchor anywhere in the docs.
  • Inbound references to the page are just docs.json:73 (sidebar) and chart-types/index.mdx:12 (index list); neither targets a section anchor, so the #inner-radius#center-total rename breaks nothing in-repo. External inbound links to the old anchor will land at the top of the page — acceptable, since the section's meaning changed entirely.
  • The removed cross-link to kpi.mdx stays removed, which is correct: it pointed at a workaround the product no longer needs.

Notes, not findings

  • The screenshot placeholders use the page's existing {/* Screenshot: … (hidden — replace …) */} form rather than the {/* TODO: screenshot — … */} form in docs-mintlify/CLAUDE.md. That's a pre-existing convention on this page, carried over consistently by the diff — not something this PR introduced, and not worth churning here.
  • Lines 24 and 26 could be one paragraph, but the split reads fine: how-to first, scope caveat second.

Verification caveat (unchanged)

The factual claims — that the control is a Shape selector with Pie/Donut options, that Show total exists and is donut-only, and that the total inherits the measure's number format — still can't be verified from this repo. grep for showTotal / innerRadius across .ts/.tsx/.js returns nothing; the chart configuration UI lives in the Cube Cloud frontend. Taking these on the author's word and reviewing for docs quality, consistency, and link integrity only.

The other standing review dimensions (security, performance, error handling, test coverage) don't apply — this is a 7-line prose change to one .mdx file with no code, build step, or test surface.

· branch [`igor/cub-3186-donut-total-docs`](https://github.com/cube-js/cube/tree/igor/cub-3186-donut-total-docs)

@igorlukanin
igorlukanin force-pushed the igor/cub-3186-donut-total-docs branch from 9f6a395 to 9d39c7d Compare August 14, 2026 21:54
@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 36s —— View job


Re-review after 9d39c7d — content identical to the approved state, no new findings

Verdict: approve. 0 high, 0 medium, 0 low. No new inline comments; my one prior thread stays resolved.

Full review — todos, findings, and notes

Todos

  • Re-read diff (git diff origin/master...HEAD) and the full file at 9d39c7d
  • Check prior review threads for staleness / dedup
  • Re-verify link and anchor integrity
  • Post review

What changed

Nothing in content. 9d39c7d ("docs: trim the center total section per review") is the rewritten form of the previously-reviewed 9f6a395 — the tree-level diff against origin/master is byte-for-byte what I approved last pass:

  • ### Donut (line 18) — Shape selector with Pie / Donut, replacing the nonexistent Inner radius slider.
  • ## Center total (line 22) — conceptual heading, Show total bolded in prose.
  • Line 24 — The total uses the measure's number format. (redundant so-clause gone).
  • Line 26 — The option appears only for a donut, since a pie has no hollow center to fill. (state-retention sentence gone).

Net +7/-5 on one .mdx.

Prior threads

One thread, PRRT_kwDOCOH15M6YGcE3 (my state-retention nit) — already isResolved: true and isOutdated: true, resolved by the author. Nothing to re-resolve or re-affirm. No unresolved Claude threads remain, so no dedup skips to report.

Link and anchor integrity

Re-verified on the current tree:

  • grep -riE "inner.?radius" across docs-mintlify/ returns nothing — no stale references to the removed control or its anchor.
  • Inbound references to this page are docs.json:73 (sidebar) and chart-types/index.mdx:12 (index list). Neither targets a section anchor, so the #inner-radius#center-total rename breaks nothing in-repo. External inbound links to the old anchor land at the top of the page — acceptable, the section's meaning changed entirely.
  • The removed cross-link to kpi.mdx stays removed, correctly: it pointed at a workaround the product no longer needs.

Notes, not findings

  • Screenshot placeholders use this page's existing {/* Screenshot: … (hidden — replace …) */} form rather than the {/* TODO: screenshot — … */} form in docs-mintlify/CLAUDE.md. Pre-existing page convention, carried over consistently — not introduced here, not worth churning.

Verification caveat (unchanged)

The factual claims — that the control is a Shape selector with Pie/Donut options, that Show total exists and is donut-only, and that the total inherits the measure's number format — can't be verified from this repo. grep for showTotal / innerRadius across .ts/.tsx/.js returns nothing; the chart configuration UI lives in the Cube Cloud frontend. Taking these on the author's word and reviewing for docs quality, consistency, and link integrity only.

The other standing review dimensions (security, performance, error handling, test coverage) don't apply — this is a 7-line prose change to one .mdx file with no code, build step, or test surface.

· branch [`igor/cub-3186-donut-total-docs`](https://github.com/cube-js/cube/tree/igor/cub-3186-donut-total-docs)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant