This is an automated email from the ASF dual-hosted git repository. sadpandajoe pushed a commit to branch fix-deckgl-legend-position-none in repository https://gitbox.apache.org/repos/asf/superset.git
commit 00b5744081065b5b11469fef7a4c9f5830937dd1 Author: sadpandajoe <[email protected]> AuthorDate: Thu Jul 23 23:38:54 2026 +0000 fix(deckgl): hide legend when Legend Position is None The Legend hide gate only matched a strict null position, but the modernized Select no longer round-trips the "None" choice (whose value is null) back as null, handing the layer undefined instead. With the component also defaulting an unset position to 'tr', "None" left the legend visible on every deck.gl layer that renders one (surfaced on Polygon). Drop the 'tr' default and hide the legend for any falsy position, so null, undefined, and '' all hide while valid corners still render. Adds Legend unit tests covering null/undefined/''/valid positions and makes the Polygon Legend mock honor the hide contract so its "None" assertions no longer pass vacuously. --- .../src/components/Legend.test.tsx | 38 +++++++++++- .../preset-chart-deckgl/src/components/Legend.tsx | 9 ++- .../src/layers/Polygon/Polygon.test.tsx | 71 ++++++++++++++-------- 3 files changed, 91 insertions(+), 27 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 b2934184bf0..b0cd43dcb93 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 @@ -22,7 +22,7 @@ import { createEvent, fireEvent, render, screen } from '@testing-library/react'; import '@testing-library/jest-dom'; import { supersetTheme, ThemeProvider } from '@apache-superset/core/theme'; import type { ReactElement } from 'react'; -import Legend from './Legend'; +import Legend, { type LegendProps } from './Legend'; const renderWithTheme = (component: ReactElement) => render(<ThemeProvider theme={supersetTheme}>{component}</ThemeProvider>); @@ -31,6 +31,7 @@ test('formats interval-notation labels while preserving brackets', () => { renderWithTheme( <Legend format=",.2f" + position="tr" categories={{ '[1, 81)': { enabled: true, color: [0, 0, 0] }, '[81, 212)': { enabled: true, color: [0, 0, 0] }, @@ -48,6 +49,7 @@ test('still formats legacy "a - b" delimiter labels', () => { renderWithTheme( <Legend format=",.1f" + position="tr" categories={{ '0 - 100000': { enabled: true, color: [0, 0, 0] }, '100001 - 200000': { enabled: true, color: [0, 0, 0] }, @@ -63,6 +65,7 @@ test('leaves labels untouched when no format is provided', () => { renderWithTheme( <Legend format={null} + position="tr" categories={{ '[1, 81)': { enabled: true, color: [0, 0, 0] } }} />, ); @@ -78,6 +81,7 @@ test('clicking a legend item toggles the category without triggering anchor navi renderWithTheme( <Legend format={null} + position="tr" categories={{ Positive: { enabled: true, color: [0, 255, 0] }, Negative: { enabled: true, color: [255, 0, 0] }, @@ -103,6 +107,7 @@ test('ctrl+clicking a legend item toggles the category without opening a new tab renderWithTheme( <Legend format={null} + position="tr" categories={{ cat1: { enabled: true, color: [255, 0, 0] }, cat2: { enabled: false, color: [0, 0, 255] }, @@ -124,3 +129,34 @@ test('ctrl+clicking a legend item toggles the category without opening a new tab expect(toggleCategory).toHaveBeenCalledTimes(1); expect(toggleCategory).toHaveBeenCalledWith('cat1'); }); + +// 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, ''])( + 'renders nothing when Legend Position is None (position=%p)', + position => { + renderWithTheme( + <Legend + format={null} + position={position as LegendProps['position']} + categories={{ Alpha: { enabled: true, color: [0, 0, 0] } }} + />, + ); + + expect(screen.queryByText('Alpha')).not.toBeInTheDocument(); + }, +); + +test('renders the legend for a valid corner position', () => { + renderWithTheme( + <Legend + format={null} + position="tr" + categories={{ Alpha: { enabled: true, color: [0, 0, 0] } }} + />, + ); + + 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 2885d42d2f3..5c160bd5226 100644 --- a/superset-frontend/plugins/preset-chart-deckgl/src/components/Legend.tsx +++ b/superset-frontend/plugins/preset-chart-deckgl/src/components/Legend.tsx @@ -98,7 +98,7 @@ export type LegendProps = { const Legend = ({ format: d3Format = null, forceCategorical = false, - position = 'tr', + position, categories: categoriesObject = {}, toggleCategory = () => {}, showSingleCategory = () => {}, @@ -136,7 +136,12 @@ const Legend = ({ return format(k); }; - if (Object.keys(categoriesObject).length === 0 || position === null) { + // 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) { 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 6c392c0486b..7ef01a3c88f 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,7 +18,7 @@ */ import type { ReactElement } from 'react'; // eslint-disable-next-line import/no-extraneous-dependencies -import { render, screen } from '@testing-library/react'; +import { render } 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'; @@ -53,15 +53,24 @@ jest.mock('../../utils/mapbox', () => ({ hasMapboxApiKey: () => true, })); -jest.mock('../../components/Legend', () => ({ categories, position }: any) => ( - <div - data-testid="legend" - data-categories={JSON.stringify(categories)} - data-position={position} - > - Legend Mock - </div> -)); +// 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. +jest.mock( + '../../components/Legend', + () => + ({ categories, position }: any) => + position ? ( + <div + data-testid="legend" + data-categories={JSON.stringify(categories)} + data-position={position} + > + Legend Mock + </div> + ) : null, +); const mockProps = { formData: { @@ -369,20 +378,30 @@ describe('DeckGLPolygon Error Handling and Edge Cases', () => { expect(mockGetBuckets).not.toHaveBeenCalled(); }); - test('handles null legend_position correctly', () => { - const propsWithNullLegendPosition = { - ...mockProps, - formData: { - ...mockProps.formData, - legend_position: null, - }, - }; + // 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)', + legendPosition => { + const propsWithNoLegend = { + ...mockProps, + formData: { + ...mockProps.formData, + legend_position: legendPosition, + }, + }; - renderWithTheme(<DeckGLPolygon {...propsWithNullLegendPosition} />); + const { container } = renderWithTheme( + <DeckGLPolygon {...propsWithNoLegend} />, + ); - // Legend should not be rendered when position is null - expect(screen.queryByTestId('legend')).not.toBeInTheDocument(); - }); + expect( + container.querySelector('[data-testid="legend"]'), + ).not.toBeInTheDocument(); + }, + ); }); describe('DeckGLPolygon Legend Integration', () => { @@ -421,10 +440,14 @@ describe('DeckGLPolygon Legend Integration', () => { }, }; - renderWithTheme(<DeckGLPolygon {...propsWithoutMetric} />); + const { container } = renderWithTheme( + <DeckGLPolygon {...propsWithoutMetric} />, + ); // Legend should not be rendered when no metric is defined - expect(screen.queryByTestId('legend')).not.toBeInTheDocument(); + expect( + container.querySelector('[data-testid="legend"]'), + ).not.toBeInTheDocument(); }); });
