msyavuz commented on code in PR #43820:
URL: https://github.com/apache/superset/pull/43820#discussion_r3933977085
##########
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:
"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.
##########
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:
##########
@@ -760,6 +760,8 @@ const transformProps = (
),
passedData,
theme,
+ undefined,
+ serverPagination,
Review Comment:
The AG Grid transform has a test for this pass-through but the legacy table
doesn't; worth mirroring it here.
##########
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:
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?
--
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]
