EnxDev commented on code in PR #43820:
URL: https://github.com/apache/superset/pull/43820#discussion_r3934009043
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/constants.ts:
##########
@@ -64,6 +69,16 @@ export const formattingOptions = [
},
];
+export const boundUnitOptions = [
+ { value: BoundUnit.Value, label: t('Value') },
+ { value: BoundUnit.Percent, label: t('% of column') },
+];
+
+export const percentDenominatorOptions = [
+ { value: PercentDenominator.Max, label: t('Column max') },
+ { value: PercentDenominator.Sum, label: t('Sum of magnitudes') },
Review Comment:
Agreed. Fixed in 61ae521c20: 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.
##########
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:
##########
@@ -760,6 +760,8 @@ const transformProps = (
),
passedData,
theme,
+ undefined,
+ serverPagination,
Review Comment:
Added in 61ae521c20. 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.
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -154,9 +276,155 @@ const renderOperator = ({
);
};
+const renderBoundFields = (
+ operator?: Comparator,
+ serverPagination?: boolean,
+) => {
+ const { showMin, showMax } = getBoundVisibility(operator);
+ // 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;
+ // Percentage bounds require the complete result set. Existing percentage
+ // configurations remain editable here, but formatters use automatic bounds
+ // while server pagination is enabled.
+ const boundUnitSelectOptions = serverPagination
+ ? boundUnitOptions.map(option =>
+ option.value === boundUnitOptions[1].value
+ ? { ...option, disabled: true }
+ : option,
+ )
+ : boundUnitOptions;
+
+ return (
+ <>
+ <Row gutter={12}>
+ <Col span={12}>
+ <FormItem
+ name="boundUnit"
+ label={t('Bound unit')}
+ initialValue={boundUnitOptions[0].value}
+ tooltip={
+ serverPagination
+ ? t(
+ '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. Existing percentage rules use the automatic
data range while Server pagination is enabled.',
+ )
+ : t(
+ 'Value: type the exact numbers used for coloring below. %
of column: type a percentage of the column maximum or the sum of value
magnitudes selected below, so the rule keeps working as the data changes.',
+ )
+ }
+ >
+ <Select
+ ariaLabel={t('Bound unit')}
+ options={boundUnitSelectOptions}
+ />
+ </FormItem>
+ </Col>
+ <Col span={12}>
+ <FormItem noStyle shouldUpdate={boundUnitShouldUpdate}>
+ {({ getFieldValue }: GetFieldValue) =>
+ getFieldValue('boundUnit') === boundUnitOptions[1].value ? (
+ <FormItem
+ name="percentDenominator"
+ label={t('% of')}
+ initialValue={percentDenominatorOptions[0].value}
+ >
+ <Select
+ ariaLabel={t('Percent denominator')}
+ options={percentDenominatorOptions}
+ />
+ </FormItem>
+ ) : null
+ }
+ </FormItem>
+ </Col>
+ </Row>
+ <Row gutter={12}>
+ {showMin && (
+ <Col span={showMax ? 12 : 24}>
+ <FormItem
+ name="minBound"
+ label={t('Min bound')}
+ rules={minRules}
+ dependencies={minDependencies}
+ normalize={normalizeOptionalNumber}
+ validateTrigger="onBlur"
+ tooltip={t(
+ 'Overrides the lowest value used for coloring. Leave blank to
use the lowest value in the data.',
+ )}
+ >
+ <FullWidthInputNumber />
+ </FormItem>
+ </Col>
+ )}
+ {showMax && (
+ <Col span={showMin ? 12 : 24}>
+ <FormItem
+ name="maxBound"
+ label={t('Max bound')}
+ rules={maxRules}
+ dependencies={maxDependencies}
+ normalize={normalizeOptionalNumber}
+ validateTrigger="onBlur"
+ tooltip={t(
+ 'Overrides the highest value used for coloring. Leave blank to
use the highest value in the data.',
+ )}
+ >
+ <FullWidthInputNumber />
+ </FormItem>
+ </Col>
+ )}
+ </Row>
+ </>
+ );
+};
+
+const renderDivergingFields = () => (
+ <>
+ <Row gutter={12}>
+ <Col span={24}>
+ <FormItem
+ name="centerValue"
+ label={t('Center value')}
+ rules={rulesCenterValue}
+ dependencies={centerValueDeps}
+ 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.',
Review Comment:
Called this out in 61ae521c20 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.
--
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]
