From e8fbfcd6c26b95dc9d628d07ad3a2a71e76d22b9 Mon Sep 17 00:00:00 2001 From: sadpandajoe Date: Sat, 25 Jul 2026 04:27:03 +0000 Subject: [PATCH] fix(deckgl): use 'none' sentinel so legend hides and Hex/Path stay visible Addresses review of the prior commit. Two problems remained: dropping the Legend's 'tr' default hid legends for deck_hex/deck_path (which have no Legend Position control, so position is always undefined); and the "None" choice value was null, which SelectControl does not reliably round-trip (control-state hydration applies `value ?? default`, restoring 'tr'), so the falsy gate never saw it in the real app. Give "None" a stable 'none' string sentinel, restore the 'tr' default, and hide when position is 'none' (also null/'' so charts saved under the old null choice keep hiding). Unset (undefined) keeps the 'tr' default so Hex/Path legends stay visible. Adds a SelectControl test proving 'none' round-trips through onChange, and reworks the Polygon test to assert legend_position is forwarded to the Legend instead of re-implementing the hide gate in the mock. --- .../src/components/Legend.test.tsx | 21 ++++- .../src/components/Legend.tsx | 18 +++-- .../src/layers/Polygon/Polygon.test.tsx | 79 +++++++++---------- .../src/utilities/Shared_DeckGL.tsx | 5 +- .../controls/SelectControl.test.tsx | 36 +++++++++ 5 files changed, 105 insertions(+), 54 deletions(-) diff --git a/superset-frontend/plugins/preset-chart-deckgl/src/components/Legend.test.tsx b/superset-frontend/plugins/preset-chart-deckgl/src/components/Legend.test.tsx index b0cd43dcb93..73819b638d2 100644 --- a/superset-frontend/plugins/preset-chart-deckgl/src/components/Legend.test.tsx +++ b/superset-frontend/plugins/preset-chart-deckgl/src/components/Legend.test.tsx @@ -131,10 +131,9 @@ test('ctrl+clicking a legend item toggles the category without opening a new tab }); // Regression proof for the "Legend Position: None" control not hiding the -// legend. The control's "None" choice has a null value, but the modernized -// Select can hand it back as undefined or an empty string, so the legend must -// hide for any falsy position rather than strictly matching null. -test.each([null, undefined, ''])( +// legend. "None" is the 'none' sentinel; null and '' also hide so charts saved +// under the older null-valued choice keep working. +test.each(['none', null, ''])( 'renders nothing when Legend Position is None (position=%p)', position => { renderWithTheme( @@ -160,3 +159,17 @@ test('renders the legend for a valid corner position', () => { expect(screen.getByText('Alpha')).toBeInTheDocument(); }); + +test('falls back to the top-right default when position is unset', () => { + // Layers without a Legend Position control (e.g. Hex, Path) pass an + // undefined position; the legend must still render at the default corner. + renderWithTheme( + , + ); + + expect(screen.getByText('Alpha')).toBeInTheDocument(); +}); diff --git a/superset-frontend/plugins/preset-chart-deckgl/src/components/Legend.tsx b/superset-frontend/plugins/preset-chart-deckgl/src/components/Legend.tsx index 5c160bd5226..5663cc5830f 100644 --- a/superset-frontend/plugins/preset-chart-deckgl/src/components/Legend.tsx +++ b/superset-frontend/plugins/preset-chart-deckgl/src/components/Legend.tsx @@ -89,7 +89,7 @@ const parseInterval = (label: string) => { export type LegendProps = { format: string | null; forceCategorical?: boolean; - position?: null | 'tl' | 'tr' | 'bl' | 'br'; + position?: null | 'none' | 'tl' | 'tr' | 'bl' | 'br'; categories: Record; toggleCategory?: (key: string) => void; showSingleCategory?: (key: string) => void; @@ -98,7 +98,7 @@ export type LegendProps = { const Legend = ({ format: d3Format = null, forceCategorical = false, - position, + position = 'tr', categories: categoriesObject = {}, toggleCategory = () => {}, showSingleCategory = () => {}, @@ -137,11 +137,15 @@ const Legend = ({ }; // Hide the legend when there are no categories, or when Legend Position is - // "None". The control's "None" choice has a null value, but the Select can - // round-trip it as undefined (or an empty string), so treat every falsy - // position as "hidden" rather than strictly matching null. Valid corner - // values ('tl'/'tr'/'bl'/'br') are truthy and fall through to render. - if (Object.keys(categoriesObject).length === 0 || !position) { + // "None". "None" is the 'none' sentinel from the control; null/'' are also + // treated as hidden so charts saved under the older null-valued choice keep + // working. An unset position (undefined) keeps the 'tr' default so layers + // without a Legend Position control (e.g. Hex, Path) still show their legend. + if ( + Object.keys(categoriesObject).length === 0 || + !position || + position === 'none' + ) { return null; } diff --git a/superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/Polygon.test.tsx b/superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/Polygon.test.tsx index 7ef01a3c88f..1776f2bd98d 100644 --- a/superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/Polygon.test.tsx +++ b/superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/Polygon.test.tsx @@ -18,11 +18,12 @@ */ import type { ReactElement } from 'react'; // eslint-disable-next-line import/no-extraneous-dependencies -import { render } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; // eslint-disable-next-line import/no-extraneous-dependencies import '@testing-library/jest-dom'; import { supersetTheme, ThemeProvider } from '@apache-superset/core/theme'; import DeckGLPolygon, { getPoints } from './Polygon'; +import type { LegendProps } from '../../components/Legend'; import { COLOR_SCHEME_TYPES } from '../../utilities/utils'; import * as utils from '../../utils'; @@ -53,23 +54,25 @@ jest.mock('../../utils/mapbox', () => ({ hasMapboxApiKey: () => true, })); -// Stand in for the real Legend while preserving its hide contract: a falsy -// position (Legend Position "None") renders nothing. Without this the mock -// would render unconditionally and the "None hides the legend" assertions -// below would pass vacuously. +// Stand in for the real Legend, exposing the props it received so the layer's +// wiring can be asserted. The hide-on-"none" behavior belongs to the real +// Legend (covered in Legend.test.tsx), so the mock does not reimplement it. +// Emits data-test (the configured testIdAttribute) so screen queries resolve. jest.mock( '../../components/Legend', () => - ({ categories, position }: any) => - position ? ( -
- Legend Mock -
- ) : null, + ({ + categories, + position, + }: Pick) => ( +
+ Legend Mock +
+ ), ); const mockProps = { @@ -378,28 +381,24 @@ describe('DeckGLPolygon Error Handling and Edge Cases', () => { expect(mockGetBuckets).not.toHaveBeenCalled(); }); - // The "None" choice can reach the layer as null or (after the Select - // round-trip) undefined; either must hide the legend. queryByTestId is - // configured to match data-test, not the mock's data-testid, so assert - // against the container directly. - test.each([null, undefined])( - 'hides the legend when legend_position is None (%p)', + // The layer forwards formData.legend_position to the Legend's position prop + // unchanged; hiding for the "none" sentinel is the Legend's responsibility + // (covered in Legend.test.tsx). Asserting the wiring here avoids + // re-implementing the hide gate in the mock. + test.each(['none', 'tr'])( + 'forwards legend_position "%s" to the Legend', legendPosition => { - const propsWithNoLegend = { + const props = { ...mockProps, - formData: { - ...mockProps.formData, - legend_position: legendPosition, - }, + formData: { ...mockProps.formData, legend_position: legendPosition }, }; - const { container } = renderWithTheme( - , - ); + renderWithTheme(); - expect( - container.querySelector('[data-testid="legend"]'), - ).not.toBeInTheDocument(); + expect(screen.getByTestId('legend')).toHaveAttribute( + 'data-position', + legendPosition, + ); }, ); }); @@ -417,16 +416,16 @@ describe('DeckGLPolygon Legend Integration', () => { render({component}); test('renders legend with non-empty categories when metric and linear_palette are defined', () => { - const { container } = renderWithTheme(); + renderWithTheme(); // Verify the component renders and calls the correct bucket function expect(mockGetBuckets).toHaveBeenCalled(); expect(mockGetColorBreakpointsBuckets).not.toHaveBeenCalled(); // Verify the legend mock was rendered with non-empty categories - const legendElement = container.querySelector('[data-testid="legend"]'); - expect(legendElement).toBeTruthy(); - const categoriesAttr = legendElement?.getAttribute('data-categories'); + const legendElement = screen.getByTestId('legend'); + expect(legendElement).toBeInTheDocument(); + const categoriesAttr = legendElement.getAttribute('data-categories'); const categoriesData = JSON.parse(categoriesAttr || '{}'); expect(Object.keys(categoriesData)).toHaveLength(2); }); @@ -440,14 +439,10 @@ describe('DeckGLPolygon Legend Integration', () => { }, }; - const { container } = renderWithTheme( - , - ); + renderWithTheme(); // Legend should not be rendered when no metric is defined - expect( - container.querySelector('[data-testid="legend"]'), - ).not.toBeInTheDocument(); + expect(screen.queryByTestId('legend')).not.toBeInTheDocument(); }); }); diff --git a/superset-frontend/plugins/preset-chart-deckgl/src/utilities/Shared_DeckGL.tsx b/superset-frontend/plugins/preset-chart-deckgl/src/utilities/Shared_DeckGL.tsx index feff6b17f57..32a3ad19ce4 100644 --- a/superset-frontend/plugins/preset-chart-deckgl/src/utilities/Shared_DeckGL.tsx +++ b/superset-frontend/plugins/preset-chart-deckgl/src/utilities/Shared_DeckGL.tsx @@ -221,7 +221,10 @@ export const legendPosition = { clearable: false, default: 'tr', choices: [ - [null, t('None')], + // A stable string sentinel rather than null: the Select control does not + // reliably round-trip a null-valued option back as null (it can surface + // as undefined/the default), which left "None" unable to hide the legend. + ['none', t('None')], ['tl', t('Top left')], ['tr', t('Top right')], ['bl', t('Bottom left')], diff --git a/superset-frontend/src/explore/components/controls/SelectControl.test.tsx b/superset-frontend/src/explore/components/controls/SelectControl.test.tsx index c599309c117..ee53ab5a831 100644 --- a/superset-frontend/src/explore/components/controls/SelectControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/SelectControl.test.tsx @@ -472,3 +472,39 @@ describe('SelectControl', () => { }); }); }); + +// Control-path regression proof for the deck.gl "Legend Position: None" bug: +// a string sentinel ('none') survives selection through the real Select and +// reaches onChange unchanged, so it can hide the legend. A null-valued option +// did not round-trip reliably, which is why the choice value is a sentinel. +test('selecting a string "none" option round-trips through onChange', async () => { + const onChange = jest.fn(); + render( + , + ); + + const selectorInput = screen.getByLabelText('Legend Position', { + selector: 'input', + }); + userEvent.click(selectorInput); + act(() => jest.runAllTimers()); + + userEvent.click(screen.getByRole('option', { name: 'None' })); + act(() => jest.runAllTimers()); + + expect(onChange).toHaveBeenCalledWith('none', expect.anything()); +});