EnxDev commented on code in PR #43938:
URL: https://github.com/apache/superset/pull/43938#discussion_r4087315352
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -243,6 +412,157 @@ describe('plugin-chart-table', () => {
expect(comparisonColumns.some(col => col.label === '%')).toBe(true);
});
+ test('should label percent-metric time comparison groups from verboseMap',
() => {
+ const transformedProps = transformProps({
+ ...testData.comparison,
+ datasource: {
+ ...testData.comparison.datasource,
+ verboseMap: {
+ metric_1: 'Metric 1',
+ percent_metric_1: 'Percent Metric 1',
+ },
+ },
+ queriesData: [
+ {
+ ...testData.comparison.queriesData[0],
+ data: [
+ {
+ metric_1: 100,
+ metric_2: 200,
+ '%percent_metric_1': 0.5,
+ date: '2023-01-01',
+ },
+ ],
+ colnames: ['metric_1', 'metric_2', '%percent_metric_1', 'date'],
+ coltypes: [
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ GenericDataType.Temporal,
+ ],
+ },
+ testData.comparison.queriesData[1],
+ ],
+ });
+
+ expect(
+ transformedProps.headerGroups?.find(
+ group => group.id === 'time-compare-metric_1',
+ )?.label,
+ ).toBe('Metric 1');
+ expect(
+ transformedProps.headerGroups?.find(
+ group => group.id === 'time-compare-%percent_metric_1',
+ )?.label,
+ ).toBe('%Percent Metric 1');
+ });
+
+ test('should not create time comparison header groups for non-numeric
metrics', () => {
+ const transformedProps = transformProps({
+ ...testData.comparison,
+ rawFormData: {
+ ...testData.comparison.rawFormData,
+ metrics: ['metric_1', 'name_metric'],
+ percent_metrics: [],
+ header_groups: [
+ {
+ id: 'time-compare-name_metric',
+ label: 'name_metric',
+ columns: [
+ 'Main name_metric',
+ '# name_metric',
+ '△ name_metric',
+ '% name_metric',
+ ],
+ source: 'time_compare',
+ },
+ ],
+ },
+ queriesData: [
+ {
+ ...testData.comparison.queriesData[0],
+ data: [{ metric_1: 100, name_metric: 'alpha', date: '2023-01-01'
}],
+ colnames: ['metric_1', 'name_metric', 'date'],
+ coltypes: [
+ GenericDataType.Numeric,
+ GenericDataType.String,
+ GenericDataType.Temporal,
+ ],
+ },
+ testData.comparison.queriesData[1],
+ ],
+ });
+
+ expect(transformedProps.headerGroups?.map(group => group.id)).toEqual([
+ 'time-compare-metric_1',
+ ]);
+ });
+
+ test('should derive header groups from time comparison when header_groups
is empty', () => {
+ const transformedProps = transformProps(testData.comparison);
+
+ expect(transformedProps.headerGroups).toEqual(
+ expect.arrayContaining([
+ expect.objectContaining({
+ id: 'time-compare-metric_1',
+ source: 'time_compare',
+ }),
+ expect.objectContaining({
+ id: 'time-compare-metric_2',
+ source: 'time_compare',
+ }),
+ ]),
+ );
+ });
+
+ test('should keep renamed time comparison header groups', () => {
+ const transformedProps = transformProps({
+ ...testData.comparison,
+ rawFormData: {
+ ...testData.comparison.rawFormData,
+ header_groups: [
+ {
+ id: 'time-compare-metric_1',
+ label: 'Renamed metric',
+ columns: [
+ 'Main metric_1',
+ '# metric_1',
+ '△ metric_1',
+ '% metric_1',
+ ],
+ source: 'time_compare',
+ },
+ ],
+ },
+ });
+
+ expect(
+ transformedProps.headerGroups?.find(
+ group => group.id === 'time-compare-metric_1',
+ )?.label,
+ ).toBe('Renamed metric');
Review Comment:
This test is the one failing in `sharded-jest-tests (8)`. It still expects
the rename to survive, and after the label refresh it gets `metric_1`.
The AG Grid twin got flipped to assert the `verbose_map` label but this one
didn't. Whichever way the Name-field note goes, this test needs to match it.
##########
superset-frontend/src/explore/components/controls/HeaderGroupsControl/HeaderGroupEditor.tsx:
##########
@@ -0,0 +1,546 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useState, type ReactNode } from 'react';
+import { t } from '@apache-superset/core/translation';
+import { css, styled } from '@apache-superset/core/theme';
+import { Button, Input, Popover, Select } from '@superset-ui/core/components';
+import { Radio } from '@superset-ui/core/components/Radio';
+import { Icons } from '@superset-ui/core/components/Icons';
+import {
+ HeaderGroupColumnOption,
+ HeaderGroupConfig,
+ HeaderGroupLabelAlign,
+ HeaderGroupPlacement,
+ MAX_HEADER_GROUP_DEPTH,
+} from './types';
+import {
+ canSaveHeaderGroup,
+ createHeaderGroup,
+ normalizeSelectedColumns,
+ moveHeaderGroupAt,
+ removeHeaderGroupAt,
+ updateHeaderGroupAt,
+} from './utils';
+
+export type HeaderGroupEditorProps = {
+ group?: HeaderGroupConfig;
+ path: number[];
+ columnOptions: HeaderGroupColumnOption[];
+ usedColumns: Set<string>;
+ onChange?: (path: number[], next: HeaderGroupConfig) => void;
+ onAddChild?: (path: number[]) => void;
+ onRemove?: (path: number[]) => void;
+ onSave?: (group: HeaderGroupConfig) => void;
+ mode?: 'add' | 'edit';
+ children?: ReactNode;
+};
+
+const FormStack = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ flex-direction: column;
+ gap: ${theme.sizeUnit * 3}px;
+ min-width: ${theme.sizeUnit * 92}px;
+ `}
+`;
+
+const FieldRow = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ flex-direction: column;
+ gap: ${theme.sizeUnit}px;
+ `}
+`;
+
+const InlineFields = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ flex-wrap: nowrap;
+ align-items: flex-start;
+ gap: ${theme.sizeUnit * 3}px;
+
+ & > *:first-of-type {
+ flex: 1.4 1 auto;
+ }
+
+ & > *:last-of-type {
+ flex: 1 1 auto;
+ }
+ `}
+`;
+
+const CompactRadioGroup = styled.div`
+ ${({ theme }) => css`
+ .ant-radio-group {
+ display: flex;
+ flex-wrap: nowrap;
+ width: 100%;
+ }
+
+ .ant-radio-button-wrapper {
+ flex: 1 1 auto;
+ height: ${theme.sizeUnit * 6}px;
+ line-height: ${theme.sizeUnit * 6 - 2}px;
+ padding-inline: ${theme.sizeUnit}px;
+ font-size: ${theme.fontSizeSM}px;
+ text-align: center;
+ }
+ `}
+`;
+
+const FieldLabel = styled.span`
+ ${({ theme }) => css`
+ color: ${theme.colorTextSecondary};
+ font-size: ${theme.fontSizeSM}px;
+ `}
+`;
+
+const FieldError = styled.span`
+ ${({ theme }) => css`
+ color: ${theme.colorError};
+ font-size: ${theme.fontSizeSM}px;
+ `}
+`;
+
+const NestedCard = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ flex-direction: column;
+ gap: ${theme.sizeUnit * 2}px;
+ padding: ${theme.sizeUnit * 2}px;
+ border: 1px solid ${theme.colorBorder};
+ border-radius: ${theme.borderRadius}px;
+ `}
+`;
+
+const NestedHeader = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ align-items: center;
+ justify-content: space-between;
+ gap: ${theme.sizeUnit}px;
+ font-weight: ${theme.fontWeightStrong};
+ `}
+`;
+
+const NestedHeaderActions = styled.div`
+ display: inline-flex;
+ align-items: center;
+`;
+
+const PopoverTitleRow = styled.div`
+ display: flex;
+ align-items: center;
+ justify-content: space-between;
+ gap: ${({ theme }) => theme.sizeUnit}px;
+`;
+
+const ApplyRow = styled.div`
+ display: flex;
+ justify-content: flex-end;
+`;
+
+function SettingsToggle({
+ collapsed,
+ onToggle,
+}: {
+ collapsed: boolean;
+ onToggle: () => void;
+}) {
+ return (
+ <Button
+ buttonStyle="link"
+ buttonSize="small"
+ aria-label={collapsed ? t('Expand settings') : t('Collapse settings')}
+ onClick={event => {
+ event.preventDefault();
+ event.stopPropagation();
+ onToggle();
+ }}
+ icon={
+ collapsed ? (
+ <Icons.DownOutlined iconSize="s" />
+ ) : (
+ <Icons.UpOutlined iconSize="s" />
+ )
+ }
+ />
+ );
+}
+
+const LABEL_ALIGN_OPTIONS: { label: string; value: HeaderGroupLabelAlign }[] =
[
+ { label: t('Left'), value: 'left' },
+ { label: t('Center'), value: 'center' },
+ { label: t('Right'), value: 'right' },
+];
+
+const PLACEMENT_OPTIONS: { label: string; value: HeaderGroupPlacement }[] = [
+ { label: t('Left'), value: 'left' },
+ { label: t('Right'), value: 'right' },
+];
+
+export function getGroupTitle(path: number[]): string {
+ const numberedPath = path.map(index => index + 1).join('.');
+ return path.length === 1
+ ? t('Group %s', numberedPath)
+ : t('Subgroup %s', numberedPath);
+}
+
+function HeaderGroupForm({
+ group,
+ path,
+ columnOptions,
+ usedColumns,
+ onChange,
+ onAddChild,
+ onRemove,
+ onMove,
+ onApply,
+ showRemove = false,
+ siblingCount = 1,
+ settingsCollapsed: settingsCollapsedProp,
+ onToggleSettings,
+}: {
+ group: HeaderGroupConfig;
+ path: number[];
+ columnOptions: HeaderGroupColumnOption[];
+ usedColumns: Set<string>;
+ onChange: (path: number[], next: HeaderGroupConfig) => void;
+ onAddChild: (path: number[]) => void;
+ onRemove: (path: number[]) => void;
+ onMove: (path: number[], toIndex: number) => void;
+ onApply?: () => void;
+ showRemove?: boolean;
+ siblingCount?: number;
+ settingsCollapsed?: boolean;
+ onToggleSettings?: () => void;
+}) {
+ const availableOptions = columnOptions.filter(
+ option =>
+ (group.columns ?? []).includes(option.value) ||
+ !usedColumns.has(option.value),
+ );
+ const [localCollapsed, setLocalCollapsed] = useState(false);
+ const canSave = canSaveHeaderGroup(group);
+ const nameError =
+ !onApply && !group.label?.trim() ? t('Name is required') : undefined;
Review Comment:
Nit, take it or leave it. Only the root form in add mode gets `onApply`, so
a new subgroup shows `Name is required` in red before anyone has typed in it,
in both modes. The add-mode parent never shows it.
Passing a `showNameError` flag down from `HeaderGroupEditor` (true when
`!isAddMode`), or only showing the error once the field has been touched, would
make it consistent.
##########
superset-frontend/src/explore/components/controls/HeaderGroupsControl/HeaderGroupEditor.tsx:
##########
@@ -0,0 +1,546 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useState, type ReactNode } from 'react';
+import { t } from '@apache-superset/core/translation';
+import { css, styled } from '@apache-superset/core/theme';
+import { Button, Input, Popover, Select } from '@superset-ui/core/components';
+import { Radio } from '@superset-ui/core/components/Radio';
+import { Icons } from '@superset-ui/core/components/Icons';
+import {
+ HeaderGroupColumnOption,
+ HeaderGroupConfig,
+ HeaderGroupLabelAlign,
+ HeaderGroupPlacement,
+ MAX_HEADER_GROUP_DEPTH,
+} from './types';
+import {
+ canSaveHeaderGroup,
+ createHeaderGroup,
+ normalizeSelectedColumns,
+ moveHeaderGroupAt,
+ removeHeaderGroupAt,
+ updateHeaderGroupAt,
+} from './utils';
+
+export type HeaderGroupEditorProps = {
+ group?: HeaderGroupConfig;
+ path: number[];
+ columnOptions: HeaderGroupColumnOption[];
+ usedColumns: Set<string>;
+ onChange?: (path: number[], next: HeaderGroupConfig) => void;
+ onAddChild?: (path: number[]) => void;
+ onRemove?: (path: number[]) => void;
+ onSave?: (group: HeaderGroupConfig) => void;
+ mode?: 'add' | 'edit';
+ children?: ReactNode;
+};
+
+const FormStack = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ flex-direction: column;
+ gap: ${theme.sizeUnit * 3}px;
+ min-width: ${theme.sizeUnit * 92}px;
+ `}
+`;
+
+const FieldRow = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ flex-direction: column;
+ gap: ${theme.sizeUnit}px;
+ `}
+`;
+
+const InlineFields = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ flex-wrap: nowrap;
+ align-items: flex-start;
+ gap: ${theme.sizeUnit * 3}px;
+
+ & > *:first-of-type {
+ flex: 1.4 1 auto;
+ }
+
+ & > *:last-of-type {
+ flex: 1 1 auto;
+ }
+ `}
+`;
+
+const CompactRadioGroup = styled.div`
+ ${({ theme }) => css`
+ .ant-radio-group {
+ display: flex;
+ flex-wrap: nowrap;
+ width: 100%;
+ }
+
+ .ant-radio-button-wrapper {
+ flex: 1 1 auto;
+ height: ${theme.sizeUnit * 6}px;
+ line-height: ${theme.sizeUnit * 6 - 2}px;
+ padding-inline: ${theme.sizeUnit}px;
+ font-size: ${theme.fontSizeSM}px;
+ text-align: center;
+ }
+ `}
+`;
+
+const FieldLabel = styled.span`
+ ${({ theme }) => css`
+ color: ${theme.colorTextSecondary};
+ font-size: ${theme.fontSizeSM}px;
+ `}
+`;
+
+const FieldError = styled.span`
+ ${({ theme }) => css`
+ color: ${theme.colorError};
+ font-size: ${theme.fontSizeSM}px;
+ `}
+`;
+
+const NestedCard = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ flex-direction: column;
+ gap: ${theme.sizeUnit * 2}px;
+ padding: ${theme.sizeUnit * 2}px;
+ border: 1px solid ${theme.colorBorder};
+ border-radius: ${theme.borderRadius}px;
+ `}
+`;
+
+const NestedHeader = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ align-items: center;
+ justify-content: space-between;
+ gap: ${theme.sizeUnit}px;
+ font-weight: ${theme.fontWeightStrong};
+ `}
+`;
+
+const NestedHeaderActions = styled.div`
+ display: inline-flex;
+ align-items: center;
+`;
+
+const PopoverTitleRow = styled.div`
+ display: flex;
+ align-items: center;
+ justify-content: space-between;
+ gap: ${({ theme }) => theme.sizeUnit}px;
+`;
+
+const ApplyRow = styled.div`
+ display: flex;
+ justify-content: flex-end;
+`;
+
+function SettingsToggle({
+ collapsed,
+ onToggle,
+}: {
+ collapsed: boolean;
+ onToggle: () => void;
+}) {
+ return (
+ <Button
+ buttonStyle="link"
+ buttonSize="small"
+ aria-label={collapsed ? t('Expand settings') : t('Collapse settings')}
+ onClick={event => {
+ event.preventDefault();
+ event.stopPropagation();
+ onToggle();
+ }}
+ icon={
+ collapsed ? (
+ <Icons.DownOutlined iconSize="s" />
+ ) : (
+ <Icons.UpOutlined iconSize="s" />
+ )
+ }
+ />
+ );
+}
+
+const LABEL_ALIGN_OPTIONS: { label: string; value: HeaderGroupLabelAlign }[] =
[
+ { label: t('Left'), value: 'left' },
+ { label: t('Center'), value: 'center' },
+ { label: t('Right'), value: 'right' },
+];
+
+const PLACEMENT_OPTIONS: { label: string; value: HeaderGroupPlacement }[] = [
+ { label: t('Left'), value: 'left' },
+ { label: t('Right'), value: 'right' },
+];
+
+export function getGroupTitle(path: number[]): string {
+ const numberedPath = path.map(index => index + 1).join('.');
+ return path.length === 1
+ ? t('Group %s', numberedPath)
+ : t('Subgroup %s', numberedPath);
+}
+
+function HeaderGroupForm({
+ group,
+ path,
+ columnOptions,
+ usedColumns,
+ onChange,
+ onAddChild,
+ onRemove,
+ onMove,
+ onApply,
+ showRemove = false,
+ siblingCount = 1,
+ settingsCollapsed: settingsCollapsedProp,
+ onToggleSettings,
+}: {
+ group: HeaderGroupConfig;
+ path: number[];
+ columnOptions: HeaderGroupColumnOption[];
+ usedColumns: Set<string>;
+ onChange: (path: number[], next: HeaderGroupConfig) => void;
+ onAddChild: (path: number[]) => void;
+ onRemove: (path: number[]) => void;
+ onMove: (path: number[], toIndex: number) => void;
+ onApply?: () => void;
+ showRemove?: boolean;
+ siblingCount?: number;
+ settingsCollapsed?: boolean;
+ onToggleSettings?: () => void;
+}) {
+ const availableOptions = columnOptions.filter(
+ option =>
+ (group.columns ?? []).includes(option.value) ||
+ !usedColumns.has(option.value),
+ );
+ const [localCollapsed, setLocalCollapsed] = useState(false);
+ const canSave = canSaveHeaderGroup(group);
+ const nameError =
+ !onApply && !group.label?.trim() ? t('Name is required') : undefined;
+ const isTimeCompareGroup = group.source === 'time_compare';
+ const isTopLevel = path.length === 1;
+ const hasSubgroups = (group.children ?? []).length > 0;
+ const canCollapse = showRemove || hasSubgroups;
+ const isCollapseControlled = settingsCollapsedProp !== undefined;
+ const settingsCollapsed = isCollapseControlled
+ ? Boolean(settingsCollapsedProp)
+ : localCollapsed;
+ const showSettings = !canCollapse || !settingsCollapsed;
+ const toggleSettings =
+ onToggleSettings ?? (() => setLocalCollapsed(collapsed => !collapsed));
+
+ return (
+ <FormStack data-test="header-group-editor">
+ {showRemove && (
+ <NestedHeader>
+ <span>{getGroupTitle(path)}</span>
+ <NestedHeaderActions>
+ {canCollapse && (
+ <SettingsToggle
+ collapsed={settingsCollapsed}
+ onToggle={toggleSettings}
+ />
+ )}
+ <Button
+ buttonStyle="link"
+ buttonSize="small"
+ aria-label={t('Move group left')}
+ disabled={path[path.length - 1] === 0}
+ onClick={() => onMove(path, path[path.length - 1] - 1)}
+ icon={<Icons.LeftOutlined iconSize="s" />}
+ />
+ <Button
+ buttonStyle="link"
+ buttonSize="small"
+ aria-label={t('Move group right')}
+ disabled={path[path.length - 1] >= siblingCount - 1}
+ onClick={() => onMove(path, path[path.length - 1] + 1)}
+ icon={<Icons.RightOutlined iconSize="s" />}
+ />
+ <Button
+ buttonStyle="link"
+ buttonSize="small"
+ aria-label={t('Remove group')}
+ onClick={() => onRemove(path)}
+ icon={<Icons.DeleteOutlined iconSize="s" />}
+ />
+ </NestedHeaderActions>
+ </NestedHeader>
+ )}
+ {showSettings && (
+ <>
+ <FieldRow>
+ <FieldLabel>{t('Name')}</FieldLabel>
+ <Input
+ aria-label={t('Group name')}
+ value={group.label}
+ placeholder={t('Enter group name')}
+ status={nameError ? 'error' : undefined}
Review Comment:
My earlier note was wrong to say these groups aren't editable. Columns is
locked on an auto group, but Name still accepts input. With
`syncTimeComparisonGroups` overwriting the label and
`headerGroupsHaveSameColumns` now comparing labels, the
`ControlPanelsContainer` effect writes the `verbose_map` label straight back,
and `transformProps` does the same thing at render. So the user types a new
name, it looks accepted in the popover, and the chart keeps showing the old one.
The docs say these groups use the metric's dataset label, so locking the
field seems like the smaller fix:
```suggestion
status={nameError ? 'error' : undefined}
disabled={isTimeCompareGroup}
```
--
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]
