geido commented on code in PR #36890:
URL: https://github.com/apache/superset/pull/36890#discussion_r2658024812
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/Scatter.tsx:
##########
@@ -44,9 +44,29 @@ function setTooltipContent(
verboseMap?: Record<string, string>,
) {
const defaultTooltipGenerator = (o: JsonObject) => {
- const label =
- verboseMap?.[formData.point_radius_fixed.value] ||
- getMetricLabel(formData.point_radius_fixed?.value);
+ // Only show metric info if point_radius_fixed is metric-based
+ let metricKey = null;
+ if (
+ formData.point_radius_fixed &&
+ typeof formData.point_radius_fixed === 'object' &&
+ formData.point_radius_fixed.type === 'metric'
Review Comment:
I see we are checking this in some other places too. Would it be beneficial
to centralize this check in a utility?
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/Scatter.tsx:
##########
@@ -44,9 +44,29 @@ function setTooltipContent(
verboseMap?: Record<string, string>,
) {
const defaultTooltipGenerator = (o: JsonObject) => {
- const label =
- verboseMap?.[formData.point_radius_fixed.value] ||
- getMetricLabel(formData.point_radius_fixed?.value);
+ // Only show metric info if point_radius_fixed is metric-based
+ let metricKey = null;
+ if (
+ formData.point_radius_fixed &&
+ typeof formData.point_radius_fixed === 'object' &&
+ formData.point_radius_fixed.type === 'metric'
+ ) {
+ const metricValue = formData.point_radius_fixed.value;
+ if (typeof metricValue === 'string') {
+ metricKey = metricValue;
+ } else if (typeof metricValue === 'object' && metricValue) {
+ // Handle object metrics (adhoc or saved metrics)
+ metricKey =
+ metricValue.label || metricValue.sqlExpression || metricValue.value;
Review Comment:
Just checking, I think this is pretty common, isn't there a util for this in
the codebase already that we can leverage?
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/Scatter.tsx:
##########
@@ -44,9 +44,29 @@ function setTooltipContent(
verboseMap?: Record<string, string>,
) {
const defaultTooltipGenerator = (o: JsonObject) => {
- const label =
- verboseMap?.[formData.point_radius_fixed.value] ||
- getMetricLabel(formData.point_radius_fixed?.value);
+ // Only show metric info if point_radius_fixed is metric-based
+ let metricKey = null;
+ if (
+ formData.point_radius_fixed &&
+ typeof formData.point_radius_fixed === 'object' &&
+ formData.point_radius_fixed.type === 'metric'
+ ) {
+ const metricValue = formData.point_radius_fixed.value;
+ if (typeof metricValue === 'string') {
+ metricKey = metricValue;
+ } else if (typeof metricValue === 'object' && metricValue) {
+ // Handle object metrics (adhoc or saved metrics)
+ metricKey =
+ metricValue.label || metricValue.sqlExpression || metricValue.value;
+ }
+ }
+
+ // Normalize metricKey for verboseMap lookup
+ const lookupKey = typeof metricKey === 'string' ? metricKey : null;
+ const label = lookupKey
Review Comment:
Same goes for this, pretty common use case
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/transformUtils.ts:
##########
@@ -134,9 +134,27 @@ export function addPropertiesToFeature<T extends
Record<string, unknown>>(
}
export function getMetricLabelFromFormData(
- metric: string | { value?: string } | undefined,
+ metric:
+ | string
+ | { value?: string | number | object; type?: string }
+ | undefined,
): string | undefined {
if (!metric) return undefined;
if (typeof metric === 'string') return getMetricLabel(metric);
- return metric.value ? getMetricLabel(metric.value) : undefined;
+ // Only return metric label if it's a metric type, not a fixed value
+ if (typeof metric === 'object' && metric.type === 'metric' && metric.value) {
+ if (typeof metric.value === 'string') {
+ return getMetricLabel(metric.value);
+ }
+ if (typeof metric.value === 'object' && metric.value) {
+ // Handle object metrics (adhoc or saved metrics)
+ const metricObj = metric.value as any;
+ const metricKey =
+ metricObj.label || metricObj.sqlExpression || metricObj.value;
Review Comment:
There is an opportunity for drying up, either by re-using existing utils if
any across the codebase, or creating one for this.
--
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]
