feat(conditional-formatting): add manual min/max color-range bounds - #43820
feat(conditional-formatting): add manual min/max color-range bounds#43820EnxDev wants to merge 27 commits into
Conversation
…orFunction Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… formatting popover
…ld validators Swap the compare-function parameter order in minBoundValidator/maxBoundValidator so targetValueValidator's actual call signature (compare(otherFieldValue, ownFieldValue)) is honored, matching the working targetValueLeftValidator/ targetValueRightValidator pattern. Previously a valid pair (e.g. min=5, max=10) was rejected while an invalid pair (min=10, max=5) passed silently. Adds two tests that type into Min bound/Max bound, blur to trigger validation, and assert on the resulting error state for both a valid and an invalid pair.
…r, fix vacuous tests, fix falsy-guard bug
- Redesign renderBoundFields via a new getBoundVisibility(operator) helper:
None shows both Min/Max bound (with the cross-field validator), the four
directional operators (>, >=, <, <=) show only the one bound
getColorFunction actually reads for them, with no cross-field rule attached
since there is nothing to cross-validate against. shouldFormItemUpdate now
compares bound visibility directly so switching between e.g. `>` and `<`
correctly swaps which field renders.
- Fix targetValueValidator's falsy guard (!targetValue || !compareValue) to
use null/undefined checks instead, so a genuine 0 bound is no longer
silently treated as "value absent, skip validation" — this also repairs
the same latent hole for the pre-existing targetValueLeft/targetValueRight
validators.
- Fix vacuous exact-match label queries in the "hides ..." tests and the
"should display tooltip icon when extraColorChoices is provided" test
(rescoped to the Color scheme field, matching its two already-fixed
siblings, with an explicit toBeInTheDocument() check before use); factor
the repeated `{ exact: false }` label-query pattern into shared
boundLabelOptions/getBoundInputs test helpers.
- Update/add tests for the corrected per-operator bound visibility (only Max
bound for `>`, only Min bound for `<`), and add a round-trip test
confirming typed Min/Max bound values reach onChange under the literal
`minBound`/`maxBound` field names Task 1's getColorFunction reads.
- Add a docs sentence on Min bound / Max bound to the conditional formatting
section of creating-your-first-dashboard.mdx.
Code Review Agent Run #10af74Actionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
github-actions
Bot
added
doc
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (99.97%) is below the target coverage (100.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #43820 +/- ##
==========================================
+ Coverage 79.42% 79.43% +0.01%
==========================================
Files 2895 2895
Lines 167997 168057 +60
Branches 38903 38982 +79
==========================================
+ Hits 133437 133504 +67
+ Misses 32061 32053 -8
- Partials 2499 2500 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #9aec91Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…for the None operator
…tDivergingColor call site
… pickers to the formatting popover
… prove per-field wiring The previous version of this test only asserted each of lowColor/midColor/ highColor was "some string" after clicking a swatch on each picker. Since none of the three ColorPickerControls pass distinct presets, clicking "the first swatch" on all three picked the identical default color three times, so the test could not have caught a swap between the three fields' `name` props. Rewritten to click a different preset index per field and assert the onChange payload against the exact color each click resolved to.
The fix round that renamed pickFirstPresetColor to pickPresetColorAt appended a new doc comment without removing the old one, leaving a paragraph describing the deleted helper's single-swatch behavior next to a verbatim-duplicated paragraph, sitting above the actual code.
…und/centerValue Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…selects to the formatting popover
… percent-bound resolution Math.max(...array) throws RangeError past ~125k elements; the Sum denominator already used a safe reduce, so switch Max to match. Also move the numeric-values filter inside the percent-mode guard so rules that never use this feature don't pay its cost.
Code Review Agent Run #786333Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #200270Actionable Suggestions - 0Additional Suggestions - 3
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…o Column max percentDenominator had no initialValue, so switching Bound unit to "% of column" left the denominator select blank even though getColorFunction already treats an unset percentDenominator as Column max. Make the default explicit in the UI (matching how boundUnit already gets an initialValue) instead of only in the resolver. Flagged in external PR review.
…nation resolvePercentBound computes its sum/max denominator from whatever rows are currently loaded. Under server pagination that's one page at a time (AG Grid Table and legacy Table both build a distinct row_limit/row_offset query per page), so the same rule would resolve a different denominator -- and therefore different colors -- on each page, defeating the whole point of "bounds" (keeping the scale fixed against exactly this kind of data-window instability). Thread a serverPagination flag from each plugin's control panel (where the server_pagination control's value already lives) down through ConditionalFormattingControl -> FormattingPopover -> FormattingPopoverContent, and disable the "% of column" Bound unit option in that mode rather than let it silently produce inconsistent colors. An already-saved config keeps working and stays visible, since disabling an option only blocks new selections. Flagged in external PR review.
…semantics resolvePercentBound previously used the raw signed sum/max as the denominator. A negative denominator (e.g. an all-losses column) could resolve minBound above maxBound, which the None branch's own guard then treats as an invalid range and silently disables coloring entirely. A zero denominator (e.g. a column summing to zero) collapsed both bounds onto the same point, which getOpacity's cutoffValue===extremeValue guard turns into full opacity for every value -- coloring the whole column at maximum intensity regardless of its actual spread. Take the absolute magnitude of the denominator so the resolved bounds stay correctly ordered, and treat a zero denominator the same as "no numeric values" (bound stays unset, falling back to the column's own data-derived range) instead of producing a degenerate single-point scale. Flagged in external PR review.
… active The diverging low/mid/high scale always interpolates a color -- it has no solid, non-gradient rendering to fall back to -- so the "Use gradient" checkbox had no effect on it. It stayed visible regardless, so a user could uncheck it while a complete diverging config was set and see no change, with no indication why. Watch the diverging fields via Form.useWatch (added to the shared Form wrapper, matching its existing useForm/Item/List exposure) and hide the checkbox once a config counts as diverging by the same field-presence check getColorFunction uses, rather than gate diverging on the checkbox itself: silently reverting a fully-configured three-color rule back to plain opacity coloring because of an unrelated, easily-left-over checkbox state would be more surprising, not less. Flagged in external PR review.
Tighten a few comments left overly long from the review-fix round, and drop a comment's reference to the review finding it addressed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
amaannawab923
left a comment
amaannawab923
left a comment
There was a problem hiding this comment.
Had a proper read through this one. Nice work, the shared-util approach means all the surfaces get it for free, and a couple of the edge cases are handled better than I expected. Disabling % of column under server pagination with the tooltip explaining why is a good catch, and using reduce instead of a spread for the denominator on big columns is the right instinct.
Five things I'd want a second look at, mostly around what happens silently when a config isn't quite valid. None of them block the approach.
Also checked two things that turned out to be fine, so ignore if you already knew: Pivot's rollup totals don't pollute the percent denominator since master already narrows colorScaleRows to leaf rows, and the fractional RGB out of getDivergingColor is safe because rgbaToHex rounds and clamps.
| // % of column derives its denominator from the loaded rows, which under | ||
| // server pagination is just the current page -- disable picking it there | ||
| // so the scale doesn't drift per page. Existing saved configs keep working. | ||
| const boundUnitSelectOptions = serverPagination |
There was a problem hiding this comment.
Disabling the option here is the right call and the comment explaining it is helpful.
Thing is it's only a UI guard. If someone saved a rule with boundUnit: percent and server pagination gets switched on afterwards, the rule still hits resolvePercentBound at runtime and resolves against whatever page happens to be loaded. There's nothing in the util that knows about pagination, so the colours just quietly differ page to page, which is the exact drift this disable is meant to stop.
The comment says an already-saved config keeps working, so this might be deliberate. If it is, worth saying so out loud somewhere, because the failure is invisible. Otherwise either the util bails out of percent mode, or turning on server pagination warns about existing rules.
There was a problem hiding this comment.
Good catch. This is addressed in 3ae2f00. getColorFormatters now accepts a disablePercentBounds flag, and both Table and AG Grid pass serverPagination through to it. As a result, an already-saved percent rule is suppressed instead of being evaluated against the loaded page. The saved configuration remains visible and editable, and the tooltip explains why percent mode is unavailable under server pagination.
| } | ||
| // Use the magnitude so a negative denominator doesn't flip the scale; | ||
| // a zero denominator falls back to unset rather than collapsing it. | ||
| const denominatorValue = Math.abs( |
There was a problem hiding this comment.
The magnitude makes sense for the sum case, but Column max gets weird when every value in the column is negative.
Say the column is [-100, -5]. Max is -5, Math.abs makes it 5, so a maxBound of 100% resolves to +5. The scale ends up as [-100, 5] and every real value sits squashed in the lower part of it.
You could argue 100% of a column whose max is -5 should just be -5. Right now it's neither that nor a no-op, it's a third thing. Probably worth picking one deliberately.
There was a problem hiding this comment.
Agreed. Since the option is Column max, preserving the actual signed maximum is the least surprising behavior. Addressed in f8c9745dc3: Column sum still uses its magnitude, while Column max retains its sign. A regression test with [-100, -5] verifies that 100% resolves to -5, and the documentation makes the distinction explicit. This commit will appear on the PR after the next push.
There was a problem hiding this comment.
Follow-up in 792a456: testing the documented minBound=0 and maxBound=100 case showed that preserving a negative maximum reverses the resolved endpoints. A non-positive Column max denominator now treats the percentage bounds as unset and falls back to the ordered, data-derived range. The regression covers [-100, -5] with both 0% and 100%, and the docs describe the fallback.
| cutoffValue: targetValue!, | ||
| extremeValue: Math.max(...allValues), | ||
| extremeValue: | ||
| maxBound !== undefined && maxBound > targetValue |
There was a problem hiding this comment.
If someone sets a maxBound that isn't greater than targetValue, the bound gets dropped and it quietly falls back to Math.max(...allValues). Same at :267, and the mirror image for minBound on the < and <= branches.
Being defensive in the util is fine. It's more that from the author's side this is "I typed a bound and nothing happened". If the popover already catches it cross-field then this is just belt and braces and all good. If not, a config coming in through import or the API misbehaves with nothing surfaced anywhere.
There was a problem hiding this comment.
The popover catches this for Value mode: > and >= require maxBound > targetValue, while < and <= require minBound < targetValue. In percent mode the direct comparison is intentionally skipped because the bound and target use different units until the column denominator is known. The formatter fallback remains as a defensive path for imported or API-provided configurations. The validation behavior is included in 3ae2f00.
| typeof cutoffValue === 'number' && | ||
| typeof extremeValue === 'number' && | ||
| centerValue > cutoffValue && | ||
| centerValue < extremeValue; |
There was a problem hiding this comment.
Strict > and < here means a centerValue that lands exactly on the resolved min or max drops the whole thing back to single-hue, silently.
Easier to hit than it looks in percent mode, where the centre is computed rather than typed. A centre of 0% or 100% resolves straight onto the bound.
Falling back rather than rendering something broken is the right instinct. Just worth making sure the popover shows why, otherwise the author sets three colours, gets one, and has no idea what happened.
There was a problem hiding this comment.
Confirmed that the popover surfaces this before submission. Its center validators use strict comparisons, so centerValue must be greater than minBound and smaller than maxBound; equality at either endpoint is rejected. The formatter fallback remains defensive for imported or API-provided invalid configurations.
|
|
||
| Each rule has a **"Use gradient"** toggle: enabled applies a varying opacity (lighter = further from threshold), disabled applies a solid fill at full opacity regardless of value. | ||
|
|
||
| For numeric rules, the optional **"Min bound"** / **"Max bound"** fields let you override the auto-detected color range with fixed values instead of relying on the minimum/maximum found in the data — useful when you want consistent coloring across dashboards or data refreshes. |
There was a problem hiding this comment.
This covers Min/Max bound well, but the diverging low/mid/high scale and the % of column bound unit aren't in here.
Those two are the harder ones to work out from the popover on your own, and percent has a real constraint worth writing down somewhere permanent, namely that it's not available under server pagination.
There was a problem hiding this comment.
Added in 3ae2f00. The documentation now explains the diverging low/mid/high scale, how percent bounds use Column max or Column sum, and why percent mode is unavailable with server pagination.
msyavuz
left a comment
msyavuz
left a comment
There was a problem hiding this comment.
Feature looks good and the tests are thorough. Two things inline, plus +1 on the docs and negative-max comments already raised.
| ) => | ||
| columnConfig?.reduce( | ||
| (acc: ColorFormatters, config: ConditionalFormattingConfig) => { | ||
| if (disablePercentBounds && config.boundUnit === BoundUnit.Percent) { |
There was a problem hiding this comment.
This drops the whole rule, so turning on server pagination silently removes all coloring for that column. Better to treat the percent bounds as unset here and fall back to data min/max instead.
There was a problem hiding this comment.
Fixed in 792a456. Enabling server pagination no longer drops the formatter. For a saved percentage rule, only minBound, maxBound, and centerValue are ignored and the rule falls back to the automatic data range, preserving comparator and solid-color behavior. The shared-util regression and the AG Grid transform regression now assert that the formatter remains present. The tooltip and docs also make this fallback explicit.
| 'Value: type the exact numbers used for coloring below. % of column is unavailable with Server pagination enabled, since each page would compute a different percentage.', | ||
| ) | ||
| : t( | ||
| 'Value: type the exact numbers used for coloring below. % of column: type a percentage of the column total instead, so the rule keeps working as the data changes.', |
There was a problem hiding this comment.
"percentage of the column total" is wrong when the denominator is Column max (the default).
There was a problem hiding this comment.
Fixed in 792a456. The tooltip now says that percent bounds use the column maximum or sum selected below. The server-pagination version also explains that existing percentage rules use the automatic data range while pagination is enabled.
Code Review Agent Run #7d3163Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
did a round of manual testing on this branch with a small dataset and hit a few cases worth a look. all of these are on the table chart, so they go through the shared 1. a column of all negative values gets no colour at all same formatting rule on both columns, gradient with percent bounds, 0 to 100 percent of max. 
2. percent of sum saturates when the signed sum lands near zero profit by region, same gradient but with 
the signed sum here is 1, so 100 percent resolves to a bound of 1 and both positive rows saturate to the same red. the 25 percent gap between north and east is invisible, and the two negative rows read as blank cells rather than as bad values. there is already a 3. center value sitting on the min or max drops the diverging config with no feedback the description calls this fallback out as intended, so this one is more of a ux note than a bug. three colour diverging, bounds 10 to 40, only center 25: 
center 40: 
the three colours are dropped and it falls back to the single hue repro data-- cases 1 and 3
SELECT 'A' AS label, -10 AS neg_col, 10 AS pos_col
UNION ALL SELECT 'B', -20, 20
UNION ALL SELECT 'C', -30, 30
UNION ALL SELECT 'D', -40, 40;
-- case 2
SELECT 'North' AS region, 500 AS profit
UNION ALL SELECT 'South', -499
UNION ALL SELECT 'East', 400
UNION ALL SELECT 'West', -400; |
|
quick follow up on the above. i had tested that against 3ae2f00 and missed the two commits that landed after it, so point 1 was already out of date when i posted. pulled the branch again and re-ran all three cases on 792a456. point 1 (all negative column gets no colour) does not reproduce anymore. the signed percent maximum change covers it. a non positive column maximum now makes the percent bound resolve to undefined and the scale falls back to the automatic data range, so the column renders a normal gradient instead of nothing. points 2 and 3 still reproduce on the latest head, re-checked just now:
sorry for the noise on the first one. |
msyavuz
left a comment
msyavuz
left a comment
There was a problem hiding this comment.
Thanks, the pagination degrade and tooltip look right now. Three small things on the latest commits.
|
|
||
| export const percentDenominatorOptions = [ | ||
| { value: PercentDenominator.Max, label: t('Column max') }, | ||
| { value: PercentDenominator.Sum, label: t('Sum of magnitudes') }, |
There was a problem hiding this comment.
"Sum of magnitudes" is going to read as jargon to most authors; for the common all-positive column it's just the column sum. I'd keep Column sum as the label and mention the absolute-value handling in the tooltip.
There was a problem hiding this comment.
Agreed. Fixed in 61ae521: the author-facing label is back to Column sum. The Bound unit tooltip and documentation explain that this option adds the absolute values so mixed signs do not cancel into an unstable denominator.
| passedData, | ||
| theme, | ||
| undefined, | ||
| serverPagination, |
There was a problem hiding this comment.
The AG Grid transform has a test for this pass-through but the legacy table doesn't; worth mirroring it here.
There was a problem hiding this comment.
Added in 61ae521. The legacy Table transform test now mirrors the AG Grid coverage: with server pagination enabled, a saved percentage rule remains present and its formatter uses the automatic data range rather than the percentage bounds.
| normalize={normalizeOptionalNumber} | ||
| validateTrigger="onBlur" | ||
| tooltip={t( | ||
| 'Optional. When set together with Low color, Mid color, and High color below, colors diverge from Mid color at this value toward Low color below it and High color above it, instead of a single color fading in and out.', |
There was a problem hiding this comment.
With the sum denominator, a center of e.g. 50% only lands inside the data range when a single row holds more than half the total, so in practice it always falls back to single hue. Should center value be scoped to Column max, or at least called out here?
There was a problem hiding this comment.
Called this out in 61ae521 rather than restricting centerValue to Column max, because a sum-based center can still be valid when the configured range contains it. The Center value tooltip and documentation now state that a Column sum percentage must resolve inside the color range; otherwise the rule uses its single-color fallback.
|
@amaannawab923 thanks for re-testing against the newer head. The two remaining points are addressed on 61ae521:
|
3 participants
SUMMARY
Adds three related conditional-formatting capabilities, unblocking OpenTable’s embedded-dashboard launch:
Manual
minBoundandmaxBoundvalues Authors can fix the endpoints of a color scale instead of recalculating them from the currently loaded data. Today, the scale usesMath.minandMath.maxfrom the column, or the entered values for theBetweenfamily. As a result, the same rule can produce different colors when filters, date ranges, or row counts change.Diverging low/mid/high color scales Authors can combine an optional
centerValuewithlowColor,midColor, andhighColorto create a three-color scale, such as red → white → green. This makes it easy to see whether a value is above or below the center, rather than only how far it is from one end of the scale.Percent-of-column bounds
minBound,maxBound, andcenterValuecan be defined as percentages of the column sum or maximum instead of fixed values. This allows the rule to adapt as the underlying data changes without requiring the author to enter new values.The changes are implemented in the shared color-scale utility (
getColorFormatters.ts) and conditional-formatting popover (FormattingPopoverContent.tsx). They are therefore available to all chart types that already support conditional formatting—Table, Pivot Table, and Big Number—without plugin-specific changes.Design decisions
Bounds apply only to
None,>,<,≥, and≤. They don’t apply toEqualorNotEqual, which perform exact matches, or to theBetweenfamily, which already usestargetValueLeftandtargetValueRight. The popover only shows the bounds that affect the selected operator.Diverging mode is available only for the
Noneoperator. It activates whencenterValueand all three colors are set, and when the resolved center falls strictly between the resolved minimum and maximum. Incomplete or invalid configurations fall back to the existing single-color behavior, avoiding partially applied or broken scales.Percent-of-column supports two denominators:
Column sumandColumn max. Pivot-specific row, column, and grand totals aren’t included because these two options have consistent meanings across Table and Pivot Table and cover the ticket’s requirements.Column maxis the default, matching the behavior of data bars in Excel and Google Sheets.The bound unit—
Valueor% of columnapplies to the entire rule, includingminBound,maxBound, andcenterValue. Using one shared unit prevents conflicting configurations within the same rule.All new fields are optional and additive. Existing rules don’t contain them and continue to render as before, so no migration is required.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
p3-before.mp4
p3-after.mp4
TESTING INSTRUCTIONS
None, set Min bound / Max bound to values wider than the data's actual range, Apply — confirm colors no longer reach full intensity (scaled against the fixed range, not the data).None, set Center value plus Low/Mid/High colors — confirm the column renders as a genuine three-color gradient pivoting at the center value, not a single hue fading in and out.% of column, pickColumn sumorColumn max, set Min bound = 0 / Max bound = 100 — confirm the rule keeps coloring correctly relative to the chosen denominator, and that switching back toValuereverts to raw-number bounds.>/</≥/≤and confirm only the relevant bound shows, and that Bound unit still applies.Automated coverage:
getColorFormatters.test.ts— 84 tests (manual bounds, diverging scale incl. operator-scoping and invalid-config fallback, percent resolution against sum/max/default/empty-column, percent-resolved centerValue feeding the diverging scale)FormattingPopoverContent.test.tsx+ConditionalFormattingControl.test.tsx— 40 tests (field visibility per operator, cross-field validation, diverging color-picker independence, Bound unit/percent-denominator visibility and payload shape)ADDITIONAL INFORMATION