amaannawab923 commented on code in PR #43820:
URL: https://github.com/apache/superset/pull/43820#discussion_r3933025965
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts:
##########
@@ -129,7 +239,10 @@ export const getColorFunction = (
typeof targetValue === 'number' && value > targetValue!
? {
cutoffValue: targetValue!,
- extremeValue: Math.max(...allValues),
+ extremeValue:
+ maxBound !== undefined && maxBound > targetValue
Review 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.
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -154,9 +269,158 @@ const renderOperator = ({
);
};
+const renderBoundFields = (
+ operator?: Comparator,
+ serverPagination?: boolean,
+) => {
+ const { showMin, showMax } = getBoundVisibility(operator);
+ if (!showMin && !showMax) {
+ return null;
+ }
+ // Cross-validate min/max only when both are shown; a lone bound
+ // validates against targetValue instead, its other end of the scale.
+ const useCrossFieldRules = showMin && showMax;
+ const minRules = useCrossFieldRules ? rulesMinBound : rulesMinBoundTarget;
+ const maxRules = useCrossFieldRules ? rulesMaxBound : rulesMaxBoundTarget;
+ const minDependencies = useCrossFieldRules ? minBoundDeps : targetValueDeps;
+ const maxDependencies = useCrossFieldRules ? maxBoundDeps : targetValueDeps;
+ // % 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
Review 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.
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts:
##########
@@ -72,6 +75,58 @@ export const getOpacity = (
);
};
+const parseColorToRgb = (color: RGBColor | string) => {
+ if (typeof color === 'string') {
+ const { r, g, b } = tinycolor(color).toRgb();
+ return { r, g, b };
+ }
+ return { r: color.r, g: color.g, b: color.b };
+};
+
+export const getDivergingColor = (
+ value: number,
+ cutoffValue: number,
+ centerValue: number,
+ extremeValue: number,
+ lowColor: RGBColor | string,
+ midColor: RGBColor | string,
+ highColor: RGBColor | string,
+): string => {
+ const clampedValue = Math.min(Math.max(value, cutoffValue), extremeValue);
+ const belowCenter = clampedValue <= centerValue;
+ const from = parseColorToRgb(belowCenter ? lowColor : midColor);
+ const to = parseColorToRgb(belowCenter ? midColor : highColor);
+ const rangeStart = belowCenter ? cutoffValue : centerValue;
+ const rangeEnd = belowCenter ? centerValue : extremeValue;
+ const ratio =
+ rangeEnd === rangeStart
+ ? 1
+ : (clampedValue - rangeStart) / (rangeEnd - rangeStart);
+ return rgbaToHex({
+ r: from.r + (to.r - from.r) * ratio,
+ g: from.g + (to.g - from.g) * ratio,
+ b: from.b + (to.b - from.b) * ratio,
+ a: 1,
+ });
+};
+
+const isValidDivergingConfig = (
+ centerValue: number | undefined,
+ lowColor: RGBColor | string | undefined,
+ midColor: RGBColor | string | undefined,
+ highColor: RGBColor | string | undefined,
+ cutoffValue: number | string,
+ extremeValue: number | string,
+): centerValue is number =>
+ centerValue !== undefined &&
+ lowColor !== undefined &&
+ midColor !== undefined &&
+ highColor !== undefined &&
+ typeof cutoffValue === 'number' &&
+ typeof extremeValue === 'number' &&
+ centerValue > cutoffValue &&
+ centerValue < extremeValue;
Review 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.
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts:
##########
@@ -83,17 +138,61 @@ export const getColorFunction = (
targetValueRight,
colorScheme,
useGradient,
+ minBound: rawMinBound,
+ maxBound: rawMaxBound,
+ centerValue: rawCenterValue,
+ lowColor,
+ midColor,
+ highColor,
+ boundUnit,
+ percentDenominator,
}: ConditionalFormattingConfig,
columnValues: number[] | string[] | (boolean | null)[],
alpha?: boolean,
) => {
+ const resolvePercentBound = (bound: number | undefined) => {
+ if (boundUnit !== BoundUnit.Percent || bound === undefined) {
+ return bound;
+ }
+ const numericColumnValues = (
+ columnValues as (number | string | boolean | null)[]
+ ).filter((value): value is number => typeof value === 'number');
+ if (numericColumnValues.length === 0) {
+ return undefined;
+ }
+ // 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(
Review 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.
##########
docs/docs/using-superset/creating-your-first-dashboard.mdx:
##########
@@ -328,6 +328,8 @@ Conditional formatting rules highlight cells based on their
values. Rules can be
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.
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
