Skip to content

docs: clarify which cells define a table color scale's range - #11423

Merged
igorlukanin merged 3 commits into
masterfrom
igor/table-chart-color-scale-totals-note
Jul 31, 2026
Merged

docs: clarify which cells define a table color scale's range#11423
igorlukanin merged 3 commits into
masterfrom
igor/table-chart-color-scale-totals-note

Conversation

@igorlukanin

Copy link
Copy Markdown
Member

Summary

The table chart's color-scale section said the scale is normalized per column, but not which cells make up that range — which matters once row totals or subtotal columns are enabled, since those sit in the same columns. Spells out that totals are colored by the scale but do not define it.

@igorlukanin
igorlukanin requested a review from keydunov as a code owner July 30, 2026 05:45
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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


Review complete — docs-only, no blockers. 1 medium (incomplete coverage of totals variants), 2 low.

Full review

Scope

Single-line prose change in docs-mintlify/docs/explore-analyze/charts/chart-types/table.mdx:309, in the Color scale section. Correct site per CLAUDE.md (/docs-mintlify, not the deprecated /docs). No code, no tests, no API surface — the security / performance / test-coverage focus areas don't apply. MDX is prose-only with no components added, so nothing to break in the Mintlify build; the page is already registered in docs.json.

What's good

The gap this closes is real. The old sentence ("normalized per column — each targeted column uses its own value range") answered what unit the scale spans but not which cells feed the range, and the Totals and Subtotals sections immediately below make that ambiguity concrete. Bolding on per column is preserved, and the em-dash style matches the surrounding paragraphs.

Findings

Medium — the column-totals row is omitted. The sentence covers row totals and subtotal columns but not the column-totals row, which is the variant where a per-column scale is most obviously at stake: a grand-total cell sits inside the targeted column. Since line 315 introduces "column totals and row totals" as a pair, the omission is noticeable within the same page. Left an inline suggestion at line 309 covering all three variants. Fix this →

Low — framing for row totals / subtotals. Those are separate columns, so "colored by that scale but do not define it" only parses if a rule targeting column X also tints X's totals column. Stating that linkage would remove the inference step.

Low — the trailing clause restates rather than adds. "so the gradient still spans the underlying values rather than the totals that aggregate them" repeats the first half. The genuinely new information would be the observable consequence: a total above the column Maximum clamps to the End color. Flagged in the same inline comment, with the caveat that the table chart implementation isn't in this repo (grep for colorScale across packages/ returns nothing — it lives in the Cloud frontend), so I could not verify the clamping behavior and it should be confirmed before adopting that wording.

None of these block merge — the change is a strict improvement over the current line as written.

• branch [`igor/table-chart-color-scale-totals-note`](https://github.com/cube-js/cube/tree/igor/table-chart-color-scale-totals-note)

@mintlify

mintlify Bot commented Jul 30, 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 Jul 30, 2026, 5:46 AM

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

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

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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


Re-review of 2e9535e — feedback addressed, no blockers. 2 wording nits.

Full review
  • Fetch the new commit and re-read the changed line in context
  • Check prior review threads (the earlier thread is already resolved + outdated — nothing to resolve)
  • Verify the implementation claims where possible
  • Post feedback

What changed since the last review

2e9535e rewrites line 309 to cover all three totals variants, and the medium finding from the previous round is resolved. The correction to my earlier suggestion is the better version: pinned totals rows return early from applyConditionalFormatting, so those cells take no scale color — "colored by the scale but excluded from the range" would have been wrong for that variant. The clamping consequence and the "scale of the measure they aggregate" linkage (my two low findings) both landed.

I can't verify applyConditionalFormatting / makeAnchoredSampler / isNonDataCell from here — a repo-wide grep for all three returns nothing, confirming the table chart lives in the Cloud frontend rather than this monorepo. Taking those on the author's word.

Docs-only change on the correct site (/docs-mintlify), prose-only MDX, page already in docs.json — the security / performance / test-coverage focus areas don't apply.

Nits (inline at line 309, with a suggestion)

  • "totals rows don't take conditional formatting" — line 280 of this same section establishes Conditional formatting and Color scale as two distinct rule types, so citing one as the reason the other doesn't apply reads as a category error. "formatting rules" covers both.
  • "a total above the column Maximum takes the End color" — correct on defaults, but the section documents End anchored to a custom Number / Percentile and a Reverse color scale toggle that swaps Start and End. Clamping to the outermost stop holds in all cases.

Both are polish on an already-correct sentence. The paragraph runs three sentences of nuance now, which is at the edge of what docs-mintlify/CLAUDE.md's concision guidance wants — but each sentence carries distinct information, so I'd leave the length alone.

• branch igor/table-chart-color-scale-totals-note

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

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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


Re-review of b5205e0 — approved in substance, no findings. All prior feedback addressed.

Full review
  • Fetch the new commit and diff against origin/master
  • Re-read line 309 in the context of the whole Color scale section
  • Check prior review threads (both resolved + outdated — nothing to resolve, nothing to re-affirm)
  • Post feedback

What changed since the last review

b5205e0 takes the suggested wording verbatim. Both nits from the previous round are closed:

  • "formatting rules" replaces "conditional formatting" as the reason totals rows take no scale color. Line 280 explicitly frames Conditional formatting and Color scale as sibling rule types under the Formatting tab, so the umbrella term is the only one that doesn't read as a category error there.
  • "clamped to that stop's color" replaces "takes the End color". This survives the two configurations documented directly above it: End anchored to a custom Number / Percentile (line 294) and the Reverse color scale toggle (line 307), where naming End specifically would have been wrong or misleading.

Final state of the paragraph

Line 309 now covers all three totals variants with the right distinction between them — row totals / subtotal columns colored but non-contributing, column-totals row not colored at all. Terminology matches line 315 ("column totals and row totals") and the Subtotals section, and [totals](#totals) resolves to a heading on the same page.

Three sentences is at the upper end of what docs-mintlify/CLAUDE.md's concision guidance wants, but each carries distinct information (range source, totals-column behavior, totals-row behavior), so the length is earned.

Scope notes

Docs-only, correct site (/docs-mintlify, not the deprecated /docs), prose-only MDX with no components added, page already registered in docs.json. The security / performance / test-coverage focus areas don't apply. As in the prior rounds, the table chart implementation isn't in this monorepo — the applyConditionalFormatting / makeAnchoredSampler behavior stands on the author's verification, not mine.

Ready to merge.

• branch igor/table-chart-color-scale-totals-note

@igorlukanin
igorlukanin merged commit 57e0bc3 into master Jul 31, 2026
15 checks passed
@igorlukanin
igorlukanin deleted the igor/table-chart-color-scale-totals-note branch July 31, 2026 00:09
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