EnxDev commented on code in PR #43938: URL: https://github.com/apache/superset/pull/43938#discussion_r4076944270
########## superset-frontend/src/explore/components/controls/HeaderGroupsControl/HeaderGroupsControl.tsx: ########## @@ -0,0 +1,289 @@ +/** + * 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 { useCallback, useEffect, useMemo } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { css, styled } from '@apache-superset/core/theme'; +import { Button } from '@superset-ui/core/components'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { + DndContext, + closestCenter, + KeyboardSensor, + PointerSensor, + useSensor, + useSensors, + type DragEndEvent, +} from '@dnd-kit/core'; +import { + SortableContext, + sortableKeyboardCoordinates, + useSortable, + verticalListSortingStrategy, +} from '@dnd-kit/sortable'; +import { restrictToVerticalAxis } from '@dnd-kit/modifiers'; +import { CSS } from '@dnd-kit/utilities'; +import ControlHeader from 'src/explore/components/ControlHeader'; +import HeaderGroupEditor, { getGroupTitle } from './HeaderGroupEditor'; +import { HeaderGroupConfig, HeaderGroupsControlProps } from './types'; +import { + collectUsedHeaderGroupColumns, + headerGroupsHaveSameColumns, + moveHeaderGroup, + pruneStaleHeaderGroupColumns, + removeHeaderGroupAt, + updateHeaderGroupAt, +} from './utils'; +import { + AddControlLabel, + CaretContainer, + Label, + OptionControlContainer, +} from '../OptionControls'; + +const GroupsContainer = styled.div` + ${({ theme }) => css` + padding: ${theme.sizeUnit}px; + border: solid 1px ${theme.colorBorder}; + border-radius: ${theme.borderRadius}px; + `} +`; + +const GroupRow = styled(OptionControlContainer)` + &, + & > div { + margin-bottom: ${({ theme }) => theme.sizeUnit}px; + :last-child { + margin-bottom: 0; + } + } +`; + +const CloseButton = styled.button` + ${({ theme }) => css` + background: ${theme.colorBgLayout}; + color: ${theme.colorIcon}; + height: 100%; + width: ${theme.sizeUnit * 6}px; + border: none; + border-right: solid 1px ${theme.colorBorder}; + padding: 0; + outline: none; + border-bottom-left-radius: 3px; + border-top-left-radius: 3px; Review Comment: Nit, take it or leave it — these two want the theme token rather than a literal 3px. ```suggestion border-bottom-left-radius: ${theme.borderRadius}px; border-top-left-radius: ${theme.borderRadius}px; ``` <!-- enxdev-human:ba72da1 --> ########## superset-frontend/src/explore/components/controls/HeaderGroupsControl/HeaderGroupsControl.tsx: ########## @@ -0,0 +1,289 @@ +/** + * 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 { useCallback, useEffect, useMemo } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { css, styled } from '@apache-superset/core/theme'; +import { Button } from '@superset-ui/core/components'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { + DndContext, + closestCenter, + KeyboardSensor, + PointerSensor, + useSensor, + useSensors, + type DragEndEvent, +} from '@dnd-kit/core'; +import { + SortableContext, + sortableKeyboardCoordinates, + useSortable, + verticalListSortingStrategy, +} from '@dnd-kit/sortable'; +import { restrictToVerticalAxis } from '@dnd-kit/modifiers'; +import { CSS } from '@dnd-kit/utilities'; +import ControlHeader from 'src/explore/components/ControlHeader'; +import HeaderGroupEditor, { getGroupTitle } from './HeaderGroupEditor'; +import { HeaderGroupConfig, HeaderGroupsControlProps } from './types'; +import { + collectUsedHeaderGroupColumns, + headerGroupsHaveSameColumns, + moveHeaderGroup, + pruneStaleHeaderGroupColumns, + removeHeaderGroupAt, + updateHeaderGroupAt, +} from './utils'; +import { + AddControlLabel, + CaretContainer, + Label, + OptionControlContainer, +} from '../OptionControls'; + +const GroupsContainer = styled.div` + ${({ theme }) => css` + padding: ${theme.sizeUnit}px; + border: solid 1px ${theme.colorBorder}; + border-radius: ${theme.borderRadius}px; + `} +`; + +const GroupRow = styled(OptionControlContainer)` + &, + & > div { + margin-bottom: ${({ theme }) => theme.sizeUnit}px; + :last-child { + margin-bottom: 0; + } + } +`; + +const CloseButton = styled.button` + ${({ theme }) => css` + background: ${theme.colorBgLayout}; + color: ${theme.colorIcon}; + height: 100%; + width: ${theme.sizeUnit * 6}px; + border: none; + border-right: solid 1px ${theme.colorBorder}; + padding: 0; + outline: none; + border-bottom-left-radius: 3px; + border-top-left-radius: 3px; + + &:focus-visible { + outline: 2px solid ${theme.colorPrimary}; + outline-offset: -2px; + } + `} +`; + +const DragHandleSlot = styled.span` + display: inline-flex; + button { + cursor: ns-resize; + padding-inline: ${({ theme }) => theme.sizeUnit}px; + } +`; + +const AddGroupIcon = styled.span` + margin: auto ${({ theme }) => theme.sizeUnit}px auto 0; + vertical-align: baseline; + display: inline-flex; +`; + +function DragHandle() { + return <Icons.MenuOutlined aria-hidden className="text-primary" />; +} + +type SortableGroupRowProps = { + group: HeaderGroupConfig; + index: number; + columnOptions: NonNullable<HeaderGroupsControlProps['columnOptions']>; + usedColumns: Set<string>; + onChange: (path: number[], next: HeaderGroupConfig) => void; + onRemove: (path: number[]) => void; +}; + +function SortableGroupRow({ + group, + index, + columnOptions, + usedColumns, + onChange, + onRemove, +}: SortableGroupRowProps) { + const { + attributes, + listeners, + setNodeRef, + setActivatorNodeRef, + transform, + transition, + } = useSortable({ id: group.id }); + + return ( + <div + ref={setNodeRef} + style={{ + transform: CSS.Transform.toString(transform), + transition: transition ?? undefined, + }} + > + <GroupRow> + <DragHandleSlot ref={setActivatorNodeRef}> + <Button + buttonStyle="link" + buttonSize="small" + aria-label={t('Drag to reorder')} + icon={<DragHandle />} + {...attributes} + {...listeners} + /> + </DragHandleSlot> + <CloseButton + aria-label={t('Remove group')} + onClick={() => onRemove([index])} + > + <Icons.CloseOutlined iconSize="m" /> + </CloseButton> + <HeaderGroupEditor + mode="edit" + group={group} + path={[index]} + columnOptions={columnOptions} + usedColumns={usedColumns} + onChange={onChange} + onRemove={onRemove} + > + <OptionControlContainer withCaret> + <Label>{getGroupTitle([index])}</Label> + <CaretContainer> + <Icons.RightOutlined iconSize="m" /> + </CaretContainer> + </OptionControlContainer> + </HeaderGroupEditor> + </GroupRow> + </div> + ); +} + +export default function HeaderGroupsControl({ + value = [], + onChange, + columnOptions = [], + timeComparisonGroups: _timeComparisonGroups, + ...props +}: HeaderGroupsControlProps) { + const groups = value ?? []; + + useEffect(() => { + if (!onChange || columnOptions.length === 0) { + return; + } + const next = pruneStaleHeaderGroupColumns(groups, columnOptions); + if (!headerGroupsHaveSameColumns(groups, next)) { + onChange(next); Review Comment: This fires from an effect on mount, and a control's `onChange` goes through `Control.tsx` into `setControlValue(name, value, errors)` with no `programmatic` flag — so opening a saved chart whose groups need pruning marks it as having unsaved changes the user never made. The sync effect you added in `ControlPanelsContainer` passes `{ programmatic: true }` for exactly this reason, and it's also the call that can quietly drop a column from a group, so the chart can change on open with no prompt. Not a one-liner given the `onChange` signature, but could the prune either route through something that can set the flag, or wait for a user interaction? <!-- enxdev-human:ba72da1 --> ########## superset-frontend/src/explore/components/controls/HeaderGroupsControl/utils.ts: ########## @@ -0,0 +1,219 @@ +/** + * 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 { nanoid } from 'nanoid'; +import { + expandGroupColumnKey, + getTimeComparisonColumnKeys, + headerGroupsHaveSameColumns, + syncTimeComparisonGroups, + toStoredTimeComparisonColumnKey, +} from '@superset-ui/chart-controls'; +import { HeaderGroupColumnOption, HeaderGroupConfig } from './types'; + +export { headerGroupsHaveSameColumns, syncTimeComparisonGroups }; + +export function createHeaderGroup(): HeaderGroupConfig { + return { + id: nanoid(), + label: '', + columns: [], + labelAlign: 'center', + placement: 'right', + children: [], + }; +} + +export function canSaveHeaderGroup(group: HeaderGroupConfig): boolean { + return ( + Boolean(group.label?.trim()) && + (group.columns ?? []).length > 0 && + (group.children ?? []).every(canSaveHeaderGroup) + ); +} + +export function moveHeaderGroup( + groups: HeaderGroupConfig[], + fromIndex: number, + toIndex: number, +): HeaderGroupConfig[] { + if ( + fromIndex === toIndex || + fromIndex < 0 || + toIndex < 0 || + fromIndex >= groups.length || + toIndex >= groups.length + ) { + return groups; + } + const next = [...groups]; + const [moved] = next.splice(fromIndex, 1); + next.splice(toIndex, 0, moved); + return next; +} + +export function normalizeSelectedColumns(columns: unknown): string[] { + return (Array.isArray(columns) ? columns : []).map(column => + typeof column === 'object' && column !== null && 'value' in column + ? String((column as { value: unknown }).value) + : String(column), + ); +} + +export function collectHeaderGroupColumns( + groups: HeaderGroupConfig[] = [], +): string[] { + return groups.flatMap(group => [ + ...(group.columns ?? []), + ...collectHeaderGroupColumns(group.children), + ]); +} + +function getClaimedColumnKeys(identifier: string): string[] { + return [ + identifier, + `%${identifier}`, Review Comment: `%${identifier}` means picking metric `revenue` also claims the percent metric `%revenue`, which is a separate metric with its own entry in the picker — so once `revenue` is in one group, `%revenue` can't be put in any other. `expandGroupColumnKey` only falls back to the `%` form when the exact key is missing, so the claim here is wider than what actually ends up grouped. Could this drop `%${identifier}` and keep just the comparison keys, with a case in `collectUsedHeaderGroupColumns` covering both `revenue` and `%revenue` in `columnOptions`? <!-- enxdev-human:ba72da1 --> -- 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]
