Copilot commented on code in PR #39434: URL: https://github.com/apache/superset/pull/39434#discussion_r3887604387
########## superset-frontend/src/dashboard/components/PropertiesModal/sections/LabelColorMapping.tsx: ########## @@ -0,0 +1,477 @@ +/** + * 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 { useEffect, useMemo, useRef, useState } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { styled } from '@apache-superset/core/theme'; +import stringify from 'json-stringify-pretty-compact'; +import ColorPickerControl from 'src/explore/components/controls/ColorPickerControl'; + +const Container = styled.div` + ${({ theme }) => ` + margin-bottom: ${theme.sizeUnit * 4}px; + padding: ${theme.sizeUnit * 4}px; + background-color: ${theme.colorFillAlter}; + border-radius: ${theme.borderRadius}px; + `} +`; + +const HeaderRow = styled.div` + ${({ theme }) => ` + display: flex; + justify-content: space-between; + align-items: center; + margin-bottom: ${theme.sizeUnit * 4}px; + `} +`; + +const SectionTitle = styled.h4` + ${({ theme }) => ` + margin-bottom: ${theme.sizeUnit * 4}px; + margin-top: 0; + `} +`; + +const SectionDescription = styled.p` + ${({ theme }) => ` + margin: 0; + font-size: 12px; + color: ${theme.colorTextSecondary}; + `} +`; + +const ErrorDescription = styled.p` + ${({ theme }) => ` + margin: 0; + font-size: 12px; + color: ${theme.colorError}; + `} +`; + +const EmptyState = styled.p` + ${({ theme }) => ` + font-style: italic; + color: ${theme.colorTextTertiary}; + `} +`; + +const Row = styled.div` + ${({ theme }) => ` + display: flex; + align-items: center; + margin-bottom: ${theme.sizeUnit * 2}px; + gap: ${theme.sizeUnit * 4}px; + `} +`; + +const InputGroup = styled.div` + display: flex; + align-items: center; + flex: 1; + min-width: 0; +`; + +const StyledInput = styled.input` + ${({ theme }) => ` + flex: 1; + width: 100%; + min-width: 0; + height: 32px; + padding: 4px 11px; + border: 1px solid ${theme.colorBorder}; + border-right: none; + border-radius: ${theme.borderRadius}px 0 0 ${theme.borderRadius}px; + color: ${theme.colorText}; + background-color: ${theme.colorBgContainer}; + outline: none; + + &:focus { + border-color: ${theme.colorPrimary}; + } + `} +`; + +const ColorPickerWrapper = styled.div` + ${({ theme }) => ` + display: flex; + align-items: center; + padding-left: ${theme.sizeUnit * 2}px; + `} +`; + +const ActionButton = styled.button` + ${({ theme }) => ` + display: flex; + align-items: center; + justify-content: center; + width: 32px; + height: 32px; + background: transparent; + border: none; + color: ${theme.colorTextSecondary}; + cursor: pointer; + padding: 0; + border-radius: ${theme.borderRadius}px; + transition: + color 0.2s, + background-color 0.2s; + + &:hover { + color: ${theme.colorError}; + background-color: ${theme.colorFillAlter}; + } + + &:focus-visible { + outline: 2px solid ${theme.colorPrimary}; + outline-offset: 2px; + } + `} +`; + +const AddMoreLink = styled.button` + ${({ theme }) => ` + background: transparent; + border: none; + padding: 0; + color: ${theme.colorPrimary}; + font-size: 14px; + font-weight: bold; + cursor: pointer; + margin-top: ${theme.sizeUnit * 2}px; + display: inline-block; + + &:hover { + text-decoration: underline; + } + + &:focus-visible { + outline: 2px solid ${theme.colorPrimary}; + outline-offset: 2px; + } + `} +`; + +interface LabelColorMappingProps { + jsonMetadata: string; + onJsonMetadataChange: (value: string) => void; +} + +interface ColorMapping { + id: string; + label: string; + color: string; +} + +type MetadataObject = Record<string, unknown>; + +const DEFAULT_NEW_COLOR = ['#0', '00000'].join(''); + +const generateId = (): string => { + if ( + typeof crypto !== 'undefined' && + typeof crypto.randomUUID === 'function' + ) { + return crypto.randomUUID(); + } + + return Math.random().toString(36).substring(2, 11); +}; + +const isValidHex = (color: unknown): color is string => + typeof color === 'string' && /^#[0-9A-Fa-f]{6}$/i.test(color); Review Comment: This rejects existing valid 3-digit CSS hex values. Dashboard metadata is already allowed to contain values such as `#000` and `#111`; `rowsFromLabelColors` replaces those with `#000000`, and editing any row then serializes the replacement, silently changing pre-existing mappings. Accept the supported short form instead of treating it as invalid. ########## superset-frontend/src/dashboard/components/PropertiesModal/sections/LabelColorMapping.tsx: ########## @@ -0,0 +1,477 @@ +/** + * 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 { useEffect, useMemo, useRef, useState } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { styled } from '@apache-superset/core/theme'; +import stringify from 'json-stringify-pretty-compact'; +import ColorPickerControl from 'src/explore/components/controls/ColorPickerControl'; + +const Container = styled.div` + ${({ theme }) => ` + margin-bottom: ${theme.sizeUnit * 4}px; + padding: ${theme.sizeUnit * 4}px; + background-color: ${theme.colorFillAlter}; + border-radius: ${theme.borderRadius}px; + `} +`; + +const HeaderRow = styled.div` + ${({ theme }) => ` + display: flex; + justify-content: space-between; + align-items: center; + margin-bottom: ${theme.sizeUnit * 4}px; + `} +`; + +const SectionTitle = styled.h4` + ${({ theme }) => ` + margin-bottom: ${theme.sizeUnit * 4}px; + margin-top: 0; + `} +`; + +const SectionDescription = styled.p` + ${({ theme }) => ` + margin: 0; + font-size: 12px; + color: ${theme.colorTextSecondary}; + `} +`; + +const ErrorDescription = styled.p` + ${({ theme }) => ` + margin: 0; + font-size: 12px; + color: ${theme.colorError}; + `} +`; + +const EmptyState = styled.p` + ${({ theme }) => ` + font-style: italic; + color: ${theme.colorTextTertiary}; + `} +`; + +const Row = styled.div` + ${({ theme }) => ` + display: flex; + align-items: center; + margin-bottom: ${theme.sizeUnit * 2}px; + gap: ${theme.sizeUnit * 4}px; + `} +`; + +const InputGroup = styled.div` + display: flex; + align-items: center; + flex: 1; + min-width: 0; +`; + +const StyledInput = styled.input` + ${({ theme }) => ` + flex: 1; + width: 100%; + min-width: 0; + height: 32px; + padding: 4px 11px; + border: 1px solid ${theme.colorBorder}; + border-right: none; + border-radius: ${theme.borderRadius}px 0 0 ${theme.borderRadius}px; + color: ${theme.colorText}; + background-color: ${theme.colorBgContainer}; + outline: none; + + &:focus { + border-color: ${theme.colorPrimary}; + } + `} +`; + +const ColorPickerWrapper = styled.div` + ${({ theme }) => ` + display: flex; + align-items: center; + padding-left: ${theme.sizeUnit * 2}px; + `} +`; + +const ActionButton = styled.button` + ${({ theme }) => ` + display: flex; + align-items: center; + justify-content: center; + width: 32px; + height: 32px; + background: transparent; + border: none; + color: ${theme.colorTextSecondary}; + cursor: pointer; + padding: 0; + border-radius: ${theme.borderRadius}px; + transition: + color 0.2s, + background-color 0.2s; + + &:hover { + color: ${theme.colorError}; + background-color: ${theme.colorFillAlter}; + } + + &:focus-visible { + outline: 2px solid ${theme.colorPrimary}; + outline-offset: 2px; + } + `} +`; + +const AddMoreLink = styled.button` + ${({ theme }) => ` + background: transparent; + border: none; + padding: 0; + color: ${theme.colorPrimary}; + font-size: 14px; + font-weight: bold; + cursor: pointer; + margin-top: ${theme.sizeUnit * 2}px; + display: inline-block; + + &:hover { + text-decoration: underline; + } + + &:focus-visible { + outline: 2px solid ${theme.colorPrimary}; + outline-offset: 2px; + } + `} +`; + +interface LabelColorMappingProps { + jsonMetadata: string; + onJsonMetadataChange: (value: string) => void; +} + +interface ColorMapping { + id: string; + label: string; + color: string; +} + +type MetadataObject = Record<string, unknown>; + +const DEFAULT_NEW_COLOR = ['#0', '00000'].join(''); + +const generateId = (): string => { + if ( + typeof crypto !== 'undefined' && + typeof crypto.randomUUID === 'function' + ) { + return crypto.randomUUID(); + } + + return Math.random().toString(36).substring(2, 11); +}; + +const isValidHex = (color: unknown): color is string => + typeof color === 'string' && /^#[0-9A-Fa-f]{6}$/i.test(color); + +const parseMetadata = ( + jsonMetadata: string, +): { + metadataObj: MetadataObject; + isValidJson: boolean; +} => { + if (!jsonMetadata.trim()) { + return { + metadataObj: {}, + isValidJson: true, + }; + } + + try { + const parsed: unknown = JSON.parse(jsonMetadata); + + if ( + parsed !== null && + typeof parsed === 'object' && + !Array.isArray(parsed) + ) { + return { + metadataObj: parsed as MetadataObject, + isValidJson: true, + }; + } + + return { + metadataObj: {}, + isValidJson: false, + }; + } catch { + return { + metadataObj: {}, + isValidJson: false, + }; + } +}; + +const getLabelColors = ( + metadataObj: MetadataObject, +): Record<string, string> => { + const value = metadataObj.label_colors; + + if (value === null || typeof value !== 'object' || Array.isArray(value)) { + return {}; + } + + return Object.fromEntries( + Object.entries(value).filter(([, color]) => typeof color === 'string'), + ); +}; + +const rowsFromLabelColors = ( + labelColors: Record<string, string>, +): ColorMapping[] => + Object.entries(labelColors).map(([label, color]) => ({ + id: generateId(), + label, + color: isValidHex(color) ? color : DEFAULT_NEW_COLOR, + })); + +const LabelColorMapping = ({ + jsonMetadata, + onJsonMetadataChange, +}: LabelColorMappingProps) => { + const { metadataObj, isValidJson } = useMemo( + () => parseMetadata(jsonMetadata), + [jsonMetadata], + ); + + const labelColors = useMemo(() => getLabelColors(metadataObj), [metadataObj]); + + const [rows, setRows] = useState<ColorMapping[]>(() => + rowsFromLabelColors(labelColors), + ); + + const lastSyncedMetadata = useRef(jsonMetadata); + + useEffect(() => { + if (lastSyncedMetadata.current === jsonMetadata) { + return; + } + + setRows(rowsFromLabelColors(labelColors)); + lastSyncedMetadata.current = jsonMetadata; + }, [jsonMetadata, labelColors]); + + const syncToJson = (currentRows: ColorMapping[]) => { + const newLabelColors: Record<string, string> = {}; + const seenLabels = new Set<string>(); + + currentRows.forEach(row => { + const trimmedLabel = row.label.trim(); + + if ( + trimmedLabel !== '' && + !seenLabels.has(trimmedLabel) && + isValidHex(row.color) + ) { + newLabelColors[trimmedLabel] = row.color; + seenLabels.add(trimmedLabel); + } + }); + + const updatedMetadata: MetadataObject = { + ...metadataObj, + label_colors: newLabelColors, + }; + + const newMetadataString = stringify(updatedMetadata); + + lastSyncedMetadata.current = newMetadataString; + onJsonMetadataChange(newMetadataString); + }; + + const handleAddRow = () => { + setRows(currentRows => [ + ...currentRows, + { + id: generateId(), + label: '', + color: DEFAULT_NEW_COLOR, + }, + ]); + }; + + const handleUpdateRow = (id: string, newLabel: string, newColor: string) => { + const newRows = rows.map(row => + row.id === id + ? { + ...row, + label: newLabel, + color: newColor, + } + : row, + ); + + setRows(newRows); + syncToJson(newRows); + }; + + const handleDeleteRow = (id: string) => { + const newRows = rows.filter(row => row.id !== id); + + setRows(newRows); + syncToJson(newRows); + }; + + const allKnownLabels = useMemo( + () => + Array.from(new Set(rows.map(row => row.label.trim()).filter(Boolean))), + [rows], + ); Review Comment: `allKnownLabels` is derived only from the rows themselves. For each row, the filter below removes every label belonging to another row, leaving only that row's current value; a newly added blank row gets no options at all. As a result, the advertised “Select or type a label” behavior never offers dashboard labels. Source this list from the dashboard chart labels (for example, the labels color map) and then exclude labels already assigned to other rows. ########## superset-frontend/src/dashboard/components/PropertiesModal/sections/StylingSection.test.tsx: ########## @@ -201,8 +200,6 @@ test('calls onShowChartTimestampsChange when switch is toggled', async () => { expect(onShowChartTimestampsChange.mock.calls[0][0]).toBe(true); }); -// CSS Template Tests -// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks describe('CSS Template functionality', () => { Review Comment: Removing the adjacent `no-restricted-globals` suppression exposes this existing `describe()` to the repository lint rule, so the changed test file will fail lint. Either flatten this block into top-level tests or retain the suppression until that migration is done. ########## superset-frontend/src/dashboard/components/PropertiesModal/sections/LabelColorMapping.tsx: ########## @@ -0,0 +1,477 @@ +/** + * 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 { useEffect, useMemo, useRef, useState } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { styled } from '@apache-superset/core/theme'; +import stringify from 'json-stringify-pretty-compact'; +import ColorPickerControl from 'src/explore/components/controls/ColorPickerControl'; + +const Container = styled.div` + ${({ theme }) => ` + margin-bottom: ${theme.sizeUnit * 4}px; + padding: ${theme.sizeUnit * 4}px; + background-color: ${theme.colorFillAlter}; + border-radius: ${theme.borderRadius}px; + `} +`; + +const HeaderRow = styled.div` + ${({ theme }) => ` + display: flex; + justify-content: space-between; + align-items: center; + margin-bottom: ${theme.sizeUnit * 4}px; + `} +`; + +const SectionTitle = styled.h4` + ${({ theme }) => ` + margin-bottom: ${theme.sizeUnit * 4}px; + margin-top: 0; + `} +`; + +const SectionDescription = styled.p` + ${({ theme }) => ` + margin: 0; + font-size: 12px; + color: ${theme.colorTextSecondary}; + `} +`; + +const ErrorDescription = styled.p` + ${({ theme }) => ` + margin: 0; + font-size: 12px; + color: ${theme.colorError}; + `} +`; + +const EmptyState = styled.p` + ${({ theme }) => ` + font-style: italic; + color: ${theme.colorTextTertiary}; + `} +`; + +const Row = styled.div` + ${({ theme }) => ` + display: flex; + align-items: center; + margin-bottom: ${theme.sizeUnit * 2}px; + gap: ${theme.sizeUnit * 4}px; + `} +`; + +const InputGroup = styled.div` + display: flex; + align-items: center; + flex: 1; + min-width: 0; +`; + +const StyledInput = styled.input` + ${({ theme }) => ` + flex: 1; + width: 100%; + min-width: 0; + height: 32px; + padding: 4px 11px; + border: 1px solid ${theme.colorBorder}; + border-right: none; + border-radius: ${theme.borderRadius}px 0 0 ${theme.borderRadius}px; + color: ${theme.colorText}; + background-color: ${theme.colorBgContainer}; + outline: none; + + &:focus { + border-color: ${theme.colorPrimary}; + } + `} +`; + +const ColorPickerWrapper = styled.div` + ${({ theme }) => ` + display: flex; + align-items: center; + padding-left: ${theme.sizeUnit * 2}px; + `} +`; + +const ActionButton = styled.button` + ${({ theme }) => ` + display: flex; + align-items: center; + justify-content: center; + width: 32px; + height: 32px; + background: transparent; + border: none; + color: ${theme.colorTextSecondary}; + cursor: pointer; + padding: 0; + border-radius: ${theme.borderRadius}px; + transition: + color 0.2s, + background-color 0.2s; + + &:hover { + color: ${theme.colorError}; + background-color: ${theme.colorFillAlter}; + } + + &:focus-visible { + outline: 2px solid ${theme.colorPrimary}; + outline-offset: 2px; + } + `} +`; + +const AddMoreLink = styled.button` + ${({ theme }) => ` + background: transparent; + border: none; + padding: 0; + color: ${theme.colorPrimary}; + font-size: 14px; + font-weight: bold; + cursor: pointer; + margin-top: ${theme.sizeUnit * 2}px; + display: inline-block; + + &:hover { + text-decoration: underline; + } + + &:focus-visible { + outline: 2px solid ${theme.colorPrimary}; + outline-offset: 2px; + } + `} +`; + +interface LabelColorMappingProps { + jsonMetadata: string; + onJsonMetadataChange: (value: string) => void; +} + +interface ColorMapping { + id: string; + label: string; + color: string; +} + +type MetadataObject = Record<string, unknown>; + +const DEFAULT_NEW_COLOR = ['#0', '00000'].join(''); + +const generateId = (): string => { + if ( + typeof crypto !== 'undefined' && + typeof crypto.randomUUID === 'function' + ) { + return crypto.randomUUID(); + } + + return Math.random().toString(36).substring(2, 11); +}; + +const isValidHex = (color: unknown): color is string => + typeof color === 'string' && /^#[0-9A-Fa-f]{6}$/i.test(color); + +const parseMetadata = ( + jsonMetadata: string, +): { + metadataObj: MetadataObject; + isValidJson: boolean; +} => { + if (!jsonMetadata.trim()) { + return { + metadataObj: {}, + isValidJson: true, + }; + } + + try { + const parsed: unknown = JSON.parse(jsonMetadata); + + if ( + parsed !== null && + typeof parsed === 'object' && + !Array.isArray(parsed) + ) { + return { + metadataObj: parsed as MetadataObject, + isValidJson: true, + }; + } + + return { + metadataObj: {}, + isValidJson: false, + }; + } catch { + return { + metadataObj: {}, + isValidJson: false, + }; + } +}; + +const getLabelColors = ( + metadataObj: MetadataObject, +): Record<string, string> => { + const value = metadataObj.label_colors; + + if (value === null || typeof value !== 'object' || Array.isArray(value)) { + return {}; + } + + return Object.fromEntries( + Object.entries(value).filter(([, color]) => typeof color === 'string'), + ); +}; + +const rowsFromLabelColors = ( + labelColors: Record<string, string>, +): ColorMapping[] => + Object.entries(labelColors).map(([label, color]) => ({ + id: generateId(), + label, + color: isValidHex(color) ? color : DEFAULT_NEW_COLOR, + })); + +const LabelColorMapping = ({ + jsonMetadata, + onJsonMetadataChange, +}: LabelColorMappingProps) => { + const { metadataObj, isValidJson } = useMemo( + () => parseMetadata(jsonMetadata), + [jsonMetadata], + ); + + const labelColors = useMemo(() => getLabelColors(metadataObj), [metadataObj]); + + const [rows, setRows] = useState<ColorMapping[]>(() => + rowsFromLabelColors(labelColors), + ); + + const lastSyncedMetadata = useRef(jsonMetadata); + + useEffect(() => { + if (lastSyncedMetadata.current === jsonMetadata) { + return; + } + + setRows(rowsFromLabelColors(labelColors)); + lastSyncedMetadata.current = jsonMetadata; + }, [jsonMetadata, labelColors]); + + const syncToJson = (currentRows: ColorMapping[]) => { + const newLabelColors: Record<string, string> = {}; + const seenLabels = new Set<string>(); + + currentRows.forEach(row => { + const trimmedLabel = row.label.trim(); + + if ( + trimmedLabel !== '' && + !seenLabels.has(trimmedLabel) && + isValidHex(row.color) + ) { + newLabelColors[trimmedLabel] = row.color; + seenLabels.add(trimmedLabel); + } + }); + + const updatedMetadata: MetadataObject = { + ...metadataObj, + label_colors: newLabelColors, + }; + + const newMetadataString = stringify(updatedMetadata); + + lastSyncedMetadata.current = newMetadataString; + onJsonMetadataChange(newMetadataString); + }; + + const handleAddRow = () => { + setRows(currentRows => [ + ...currentRows, + { + id: generateId(), + label: '', + color: DEFAULT_NEW_COLOR, + }, + ]); + }; + + const handleUpdateRow = (id: string, newLabel: string, newColor: string) => { + const newRows = rows.map(row => + row.id === id + ? { + ...row, + label: newLabel, + color: newColor, + } + : row, + ); + + setRows(newRows); + syncToJson(newRows); + }; + + const handleDeleteRow = (id: string) => { + const newRows = rows.filter(row => row.id !== id); + + setRows(newRows); + syncToJson(newRows); + }; + + const allKnownLabels = useMemo( + () => + Array.from(new Set(rows.map(row => row.label.trim()).filter(Boolean))), + [rows], + ); + + if (!isValidJson) { + return ( + <Container> + <HeaderRow> + <div> + <SectionTitle>{t('Label Colors')}</SectionTitle> + + <ErrorDescription> + {t( + 'Invalid JSON metadata. Please resolve syntax errors in the Advanced tab to use the GUI.', + )} + </ErrorDescription> + </div> + </HeaderRow> + </Container> + ); + } + + return ( + <Container> + <HeaderRow> + <div> + <SectionTitle>{t('Label Colors')}</SectionTitle> + + <SectionDescription> + {t( + 'Map specific labels to colors. This automatically updates the JSON below.', + )} + </SectionDescription> + </div> + </HeaderRow> + + {rows.length === 0 && ( + <EmptyState> + {t('No color mappings defined. Click "+ Add more" to get started.')} + </EmptyState> + )} + + {rows.map(row => { + const currentLabel = row.label.trim(); + + const availableOptions = allKnownLabels + .filter( + label => + label === currentLabel || + !rows.some( + otherRow => + otherRow.id !== row.id && otherRow.label.trim() === label, + ), + ) + .map(label => ({ + label, + value: label, + })); + + return ( + <Row key={row.id}> + <InputGroup> + <StyledInput + list={`label-options-${row.id}`} + value={row.label} + onChange={event => + handleUpdateRow(row.id, event.target.value, row.color) + } + placeholder={t('Select or type a label')} + aria-label={t('Label')} + /> + + <datalist id={`label-options-${row.id}`}> + {availableOptions.map(option => ( + <option + key={option.value} + value={option.value} + aria-label={option.value} + /> + ))} + </datalist> + + <ColorPickerWrapper> + <ColorPickerControl + value={row.color} + outputFormat="hex" Review Comment: The color picker receives no accessible name, so assistive-technology users cannot identify what this control edits (especially with multiple mapping rows). Pass a row-specific `ariaLabel` to `ColorPickerControl`. ########## superset-frontend/src/dashboard/components/PropertiesModal/sections/LabelColorMapping.test.tsx: ########## @@ -0,0 +1,71 @@ +/** + * 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 { + render, + screen, + fireEvent, + waitFor, +} from 'spec/helpers/testing-library'; +import LabelColorMapping from './LabelColorMapping'; + +const defaultProps = { + jsonMetadata: '{}', + onJsonMetadataChange: jest.fn(), +}; + +describe('LabelColorMapping', () => { Review Comment: This new suite introduces a `describe()` block even though the frontend testing convention requires top-level `test()` calls, and `describe` is restricted by lint. Flatten these tests (the `beforeEach` can remain top-level) so the new file passes the repository rule. ########## superset-frontend/src/dashboard/components/PropertiesModal/sections/LabelColorMapping.tsx: ########## @@ -0,0 +1,477 @@ +/** + * 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 { useEffect, useMemo, useRef, useState } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { styled } from '@apache-superset/core/theme'; +import stringify from 'json-stringify-pretty-compact'; +import ColorPickerControl from 'src/explore/components/controls/ColorPickerControl'; + +const Container = styled.div` + ${({ theme }) => ` + margin-bottom: ${theme.sizeUnit * 4}px; + padding: ${theme.sizeUnit * 4}px; + background-color: ${theme.colorFillAlter}; + border-radius: ${theme.borderRadius}px; + `} +`; + +const HeaderRow = styled.div` + ${({ theme }) => ` + display: flex; + justify-content: space-between; + align-items: center; + margin-bottom: ${theme.sizeUnit * 4}px; + `} +`; + +const SectionTitle = styled.h4` + ${({ theme }) => ` + margin-bottom: ${theme.sizeUnit * 4}px; + margin-top: 0; + `} +`; + +const SectionDescription = styled.p` + ${({ theme }) => ` + margin: 0; + font-size: 12px; + color: ${theme.colorTextSecondary}; + `} +`; + +const ErrorDescription = styled.p` + ${({ theme }) => ` + margin: 0; + font-size: 12px; + color: ${theme.colorError}; + `} +`; + +const EmptyState = styled.p` + ${({ theme }) => ` + font-style: italic; + color: ${theme.colorTextTertiary}; + `} +`; + +const Row = styled.div` + ${({ theme }) => ` + display: flex; + align-items: center; + margin-bottom: ${theme.sizeUnit * 2}px; + gap: ${theme.sizeUnit * 4}px; + `} +`; + +const InputGroup = styled.div` + display: flex; + align-items: center; + flex: 1; + min-width: 0; +`; + +const StyledInput = styled.input` + ${({ theme }) => ` + flex: 1; + width: 100%; + min-width: 0; + height: 32px; + padding: 4px 11px; + border: 1px solid ${theme.colorBorder}; + border-right: none; + border-radius: ${theme.borderRadius}px 0 0 ${theme.borderRadius}px; + color: ${theme.colorText}; + background-color: ${theme.colorBgContainer}; + outline: none; + + &:focus { + border-color: ${theme.colorPrimary}; + } + `} +`; + +const ColorPickerWrapper = styled.div` + ${({ theme }) => ` + display: flex; + align-items: center; + padding-left: ${theme.sizeUnit * 2}px; + `} +`; + +const ActionButton = styled.button` + ${({ theme }) => ` + display: flex; + align-items: center; + justify-content: center; + width: 32px; + height: 32px; + background: transparent; + border: none; + color: ${theme.colorTextSecondary}; + cursor: pointer; + padding: 0; + border-radius: ${theme.borderRadius}px; + transition: + color 0.2s, + background-color 0.2s; + + &:hover { + color: ${theme.colorError}; + background-color: ${theme.colorFillAlter}; + } + + &:focus-visible { + outline: 2px solid ${theme.colorPrimary}; + outline-offset: 2px; + } + `} +`; + +const AddMoreLink = styled.button` + ${({ theme }) => ` + background: transparent; + border: none; + padding: 0; + color: ${theme.colorPrimary}; + font-size: 14px; + font-weight: bold; + cursor: pointer; + margin-top: ${theme.sizeUnit * 2}px; + display: inline-block; + + &:hover { + text-decoration: underline; + } + + &:focus-visible { + outline: 2px solid ${theme.colorPrimary}; + outline-offset: 2px; + } + `} +`; + +interface LabelColorMappingProps { + jsonMetadata: string; + onJsonMetadataChange: (value: string) => void; +} + +interface ColorMapping { + id: string; + label: string; + color: string; +} + +type MetadataObject = Record<string, unknown>; + +const DEFAULT_NEW_COLOR = ['#0', '00000'].join(''); + +const generateId = (): string => { + if ( + typeof crypto !== 'undefined' && + typeof crypto.randomUUID === 'function' + ) { + return crypto.randomUUID(); + } + + return Math.random().toString(36).substring(2, 11); +}; + +const isValidHex = (color: unknown): color is string => + typeof color === 'string' && /^#[0-9A-Fa-f]{6}$/i.test(color); + +const parseMetadata = ( + jsonMetadata: string, +): { + metadataObj: MetadataObject; + isValidJson: boolean; +} => { + if (!jsonMetadata.trim()) { + return { + metadataObj: {}, + isValidJson: true, + }; + } + + try { + const parsed: unknown = JSON.parse(jsonMetadata); + + if ( + parsed !== null && + typeof parsed === 'object' && + !Array.isArray(parsed) + ) { + return { + metadataObj: parsed as MetadataObject, + isValidJson: true, + }; + } + + return { + metadataObj: {}, + isValidJson: false, + }; + } catch { + return { + metadataObj: {}, + isValidJson: false, + }; + } +}; + +const getLabelColors = ( + metadataObj: MetadataObject, +): Record<string, string> => { + const value = metadataObj.label_colors; + + if (value === null || typeof value !== 'object' || Array.isArray(value)) { + return {}; + } + + return Object.fromEntries( + Object.entries(value).filter(([, color]) => typeof color === 'string'), + ); +}; + +const rowsFromLabelColors = ( + labelColors: Record<string, string>, +): ColorMapping[] => + Object.entries(labelColors).map(([label, color]) => ({ + id: generateId(), + label, + color: isValidHex(color) ? color : DEFAULT_NEW_COLOR, + })); + +const LabelColorMapping = ({ + jsonMetadata, + onJsonMetadataChange, +}: LabelColorMappingProps) => { + const { metadataObj, isValidJson } = useMemo( + () => parseMetadata(jsonMetadata), + [jsonMetadata], + ); + + const labelColors = useMemo(() => getLabelColors(metadataObj), [metadataObj]); + + const [rows, setRows] = useState<ColorMapping[]>(() => + rowsFromLabelColors(labelColors), + ); + + const lastSyncedMetadata = useRef(jsonMetadata); + + useEffect(() => { + if (lastSyncedMetadata.current === jsonMetadata) { + return; + } + + setRows(rowsFromLabelColors(labelColors)); + lastSyncedMetadata.current = jsonMetadata; + }, [jsonMetadata, labelColors]); + + const syncToJson = (currentRows: ColorMapping[]) => { + const newLabelColors: Record<string, string> = {}; + const seenLabels = new Set<string>(); + + currentRows.forEach(row => { + const trimmedLabel = row.label.trim(); + + if ( + trimmedLabel !== '' && + !seenLabels.has(trimmedLabel) && + isValidHex(row.color) + ) { + newLabelColors[trimmedLabel] = row.color; + seenLabels.add(trimmedLabel); + } + }); + + const updatedMetadata: MetadataObject = { + ...metadataObj, + label_colors: newLabelColors, + }; + + const newMetadataString = stringify(updatedMetadata); + + lastSyncedMetadata.current = newMetadataString; + onJsonMetadataChange(newMetadataString); Review Comment: The new tests only cover rendering, adding an empty row, and initial loading; none assert that label/color edits or deletions update `jsonMetadata`, nor that an external metadata edit updates the rows. Those are the component's primary two-way-binding behaviors and need RTL coverage to catch serialization and synchronization regressions. -- 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]
