diff --git a/superset-frontend/src/dashboard/components/Dashboard.tsx b/superset-frontend/src/dashboard/components/Dashboard.tsx index 51519ba2309..ad4b205761a 100644 --- a/superset-frontend/src/dashboard/components/Dashboard.tsx +++ b/superset-frontend/src/dashboard/components/Dashboard.tsx @@ -16,8 +16,8 @@ * specific language governing permissions and limitations * under the License. */ -import { PureComponent, ReactNode } from 'react'; -import { t } from '@apache-superset/core/translation'; +import { ReactNode, useCallback, useContext, useEffect, useRef } from 'react'; +import { t } from '@apache-superset/core/ui'; import { JsonObject } from '@superset-ui/core'; import { Loading } from '@superset-ui/core/components'; @@ -90,165 +90,61 @@ interface VisibilityEventData { ts: number; } -class Dashboard extends PureComponent { - static contextType = PluginContext; +function unload(event: BeforeUnloadEvent): string { + const message = t('You have unsaved changes.'); + // Set returnValue on the actual event object to trigger the browser prompt + event.returnValue = message; + return message; // Gecko + Webkit, Safari, Chrome etc. +} - // Use type assertion when accessing context instead of declare field - // to avoid babel transformation issues in Jest - - static defaultProps = { - timeout: 60, - userId: '', - }; - - appliedFilters: ActiveFilters; - - appliedOwnDataCharts: JsonObject; - - visibilityEventData: VisibilityEventData; - - static onBeforeUnload(hasChanged: boolean): void { - if (hasChanged) { - window.addEventListener('beforeunload', Dashboard.unload); - } else { - window.removeEventListener('beforeunload', Dashboard.unload); - } +function onBeforeUnload(hasChanged: boolean): void { + if (hasChanged) { + window.addEventListener('beforeunload', unload); + } else { + window.removeEventListener('beforeunload', unload); } +} - static unload(): string { - const message = t('You have unsaved changes.'); - // Gecko + IE: returnValue is typed as boolean but historically accepts string - (window.event as BeforeUnloadEvent).returnValue = message; - return message; // Gecko + Webkit, Safari, Chrome etc. - } +function Dashboard({ + actions, + dashboardId, + editMode, + isPublished, + hasUnsavedChanges, + slices, + activeFilters, + chartConfiguration, + datasources, + ownDataCharts, + layout, + impressionId, + timeout = 60, + userId = '', + children, +}: DashboardProps): JSX.Element { + const context = useContext(PluginContext) as PluginContextType; - constructor(props: DashboardProps) { - super(props); - this.appliedFilters = props.activeFilters ?? {}; - this.appliedOwnDataCharts = props.ownDataCharts ?? {}; - this.visibilityEventData = { start_offset: 0, ts: 0 }; - this.onVisibilityChange = this.onVisibilityChange.bind(this); - } + // Use refs to track mutable values that persist across renders + const appliedFiltersRef = useRef(activeFilters ?? {}); + const appliedOwnDataChartsRef = useRef(ownDataCharts ?? {}); + const visibilityEventDataRef = useRef({ + start_offset: 0, + ts: 0, + }); + const prevLayoutRef = useRef(layout); + const prevDashboardIdRef = useRef(dashboardId); - componentDidMount(): void { - const bootstrapData = getBootstrapData(); - const { editMode, isPublished, layout } = this.props; - const eventData: Record = { - is_soft_navigation: Logger.timeOriginOffset > 0, - is_edit_mode: editMode, - mount_duration: Logger.getTimestamp(), - is_empty: isDashboardEmpty(layout), - is_published: isPublished, - bootstrap_data_length: JSON.stringify(bootstrapData).length, - }; - const directLinkComponentId = getLocationHash(); - if (directLinkComponentId) { - eventData.target_id = directLinkComponentId; - } - this.props.actions.logEvent(LOG_ACTIONS_MOUNT_DASHBOARD, eventData); - - // Handle browser tab visibility change - if (document.visibilityState === 'hidden') { - this.visibilityEventData = { - start_offset: Logger.getTimestamp(), - ts: new Date().getTime(), - }; - } - window.addEventListener('visibilitychange', this.onVisibilityChange); - this.applyCharts(); - } - - componentDidUpdate(prevProps: DashboardProps): void { - this.applyCharts(); - const currentChartIds = getChartIdsFromLayout(prevProps.layout); - const nextChartIds = getChartIdsFromLayout(this.props.layout); - - if (prevProps.dashboardId !== this.props.dashboardId) { - // single-page-app navigation check - return; - } - - if (currentChartIds.length < nextChartIds.length) { - const newChartIds = nextChartIds.filter( - key => currentChartIds.indexOf(key) === -1, - ); - newChartIds.forEach(newChartId => - this.props.actions.addSliceToDashboard( - newChartId, - getLayoutComponentFromChartId(this.props.layout, newChartId), - ), - ); - } else if (currentChartIds.length > nextChartIds.length) { - // remove chart - const removedChartIds = currentChartIds.filter( - key => nextChartIds.indexOf(key) === -1, - ); - removedChartIds.forEach(removedChartId => - this.props.actions.removeSliceFromDashboard(removedChartId), - ); - } - } - - applyCharts(): void { - const { - activeFilters, - ownDataCharts, - chartConfiguration, - hasUnsavedChanges, - editMode, - } = this.props; - const { appliedFilters, appliedOwnDataCharts } = this; - if (!chartConfiguration) { - // For a first loading we need to wait for cross filters charts data loaded to get all active filters - // for correct comparing of filters to avoid unnecessary requests - return; - } - - if ( - !editMode && - (!areObjectsEqual(appliedOwnDataCharts, ownDataCharts, { - ignoreUndefined: true, - }) || - !areObjectsEqual(appliedFilters, activeFilters, { - ignoreUndefined: true, - })) - ) { - this.applyFilters(); - } - - if (hasUnsavedChanges) { - Dashboard.onBeforeUnload(true); - } else { - Dashboard.onBeforeUnload(false); - } - } - - componentWillUnmount(): void { - window.removeEventListener('visibilitychange', this.onVisibilityChange); - this.props.actions.clearDataMaskState(); - this.props.actions.clearAllChartStates(); - } - - onVisibilityChange(): void { - if (document.visibilityState === 'hidden') { - // from visible to hidden - this.visibilityEventData = { - start_offset: Logger.getTimestamp(), - ts: new Date().getTime(), - }; - } else if (document.visibilityState === 'visible') { - // from hidden to visible - const logStart = this.visibilityEventData.start_offset; - this.props.actions.logEvent(LOG_ACTIONS_HIDE_BROWSER_TAB, { - ...this.visibilityEventData, - duration: Logger.getTimestamp() - logStart, + const refreshCharts = useCallback( + (ids: (string | number)[]): void => { + ids.forEach(id => { + actions.triggerQuery(true, id); }); - } - } + }, + [actions], + ); - applyFilters(): void { - const { appliedFilters } = this; - const { activeFilters, ownDataCharts, slices } = this.props; + const applyFilters = useCallback((): void => { + const appliedFilters = appliedFiltersRef.current; // refresh charts if a filter was removed, added, or changed @@ -258,7 +154,7 @@ class Dashboard extends PureComponent { const allKeys = new Set(currFilterKeys.concat(appliedFilterKeys)); const affectedChartIds: (string | number)[] = getAffectedOwnDataCharts( ownDataCharts, - this.appliedOwnDataCharts, + appliedOwnDataChartsRef.current, ); [...allKeys].forEach(filterKey => { @@ -321,24 +217,145 @@ class Dashboard extends PureComponent { }); // remove dup in affectedChartIds - this.refreshCharts([...new Set(affectedChartIds)]); - this.appliedFilters = activeFilters; - this.appliedOwnDataCharts = ownDataCharts; - } + refreshCharts([...new Set(affectedChartIds)]); + appliedFiltersRef.current = activeFilters; + appliedOwnDataChartsRef.current = ownDataCharts; + }, [activeFilters, ownDataCharts, slices, refreshCharts]); - refreshCharts(ids: (string | number)[]): void { - ids.forEach(id => { - this.props.actions.triggerQuery(true, id); - }); - } - - render(): ReactNode { - const context = this.context as PluginContextType; - if (context.loading) { - return ; + const applyCharts = useCallback((): void => { + if (!chartConfiguration) { + // For a first loading we need to wait for cross filters charts data loaded to get all active filters + // for correct comparing of filters to avoid unnecessary requests + return; } - return this.props.children; + + if ( + !editMode && + (!areObjectsEqual(appliedOwnDataChartsRef.current, ownDataCharts, { + ignoreUndefined: true, + }) || + !areObjectsEqual(appliedFiltersRef.current, activeFilters, { + ignoreUndefined: true, + })) + ) { + applyFilters(); + } + + if (hasUnsavedChanges) { + onBeforeUnload(true); + } else { + onBeforeUnload(false); + } + }, [ + chartConfiguration, + editMode, + ownDataCharts, + activeFilters, + hasUnsavedChanges, + applyFilters, + ]); + + const onVisibilityChange = useCallback((): void => { + if (document.visibilityState === 'hidden') { + // from visible to hidden + visibilityEventDataRef.current = { + start_offset: Logger.getTimestamp(), + ts: new Date().getTime(), + }; + } else if (document.visibilityState === 'visible') { + // from hidden to visible + const logStart = visibilityEventDataRef.current.start_offset; + actions.logEvent(LOG_ACTIONS_HIDE_BROWSER_TAB, { + ...visibilityEventDataRef.current, + duration: Logger.getTimestamp() - logStart, + }); + } + }, [actions]); + + // componentDidMount equivalent + useEffect(() => { + const bootstrapData = getBootstrapData(); + const eventData: Record = { + is_soft_navigation: Logger.timeOriginOffset > 0, + is_edit_mode: editMode, + mount_duration: Logger.getTimestamp(), + is_empty: isDashboardEmpty(layout), + is_published: isPublished, + bootstrap_data_length: JSON.stringify(bootstrapData).length, + }; + const directLinkComponentId = getLocationHash(); + if (directLinkComponentId) { + eventData.target_id = directLinkComponentId; + } + actions.logEvent(LOG_ACTIONS_MOUNT_DASHBOARD, eventData); + + // Handle browser tab visibility change + if (document.visibilityState === 'hidden') { + visibilityEventDataRef.current = { + start_offset: Logger.getTimestamp(), + ts: new Date().getTime(), + }; + } + window.addEventListener('visibilitychange', onVisibilityChange); + + // componentWillUnmount equivalent + return () => { + window.removeEventListener('visibilitychange', onVisibilityChange); + onBeforeUnload(false); // Remove beforeunload listener on unmount + actions.clearDataMaskState(); + actions.clearAllChartStates(); + }; + // Only run on mount/unmount - intentionally excluding deps that would cause re-runs + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + + // Apply charts on every render (like componentDidMount + componentDidUpdate calling applyCharts) + useEffect(() => { + applyCharts(); + }, [applyCharts]); + + // componentDidUpdate equivalent for layout changes + useEffect(() => { + const prevLayout = prevLayoutRef.current; + const prevDashboardId = prevDashboardIdRef.current; + + // Update refs for next comparison + prevLayoutRef.current = layout; + prevDashboardIdRef.current = dashboardId; + + const currentChartIds = getChartIdsFromLayout(prevLayout); + const nextChartIds = getChartIdsFromLayout(layout); + + if (prevDashboardId !== dashboardId) { + // single-page-app navigation check + return; + } + + if (currentChartIds.length < nextChartIds.length) { + const newChartIds = nextChartIds.filter( + key => currentChartIds.indexOf(key) === -1, + ); + newChartIds.forEach(newChartId => + actions.addSliceToDashboard( + newChartId, + getLayoutComponentFromChartId(layout, newChartId), + ), + ); + } else if (currentChartIds.length > nextChartIds.length) { + // remove chart + const removedChartIds = currentChartIds.filter( + key => nextChartIds.indexOf(key) === -1, + ); + removedChartIds.forEach(removedChartId => + actions.removeSliceFromDashboard(removedChartId), + ); + } + }, [layout, dashboardId, actions]); + + if (context.loading) { + return ; } + return <>{children}; } export default Dashboard; diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/utils.ts b/superset-frontend/src/dashboard/components/DashboardBuilder/utils.ts index 8ba5405bf30..6b5836544ed 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/utils.ts +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/utils.ts @@ -32,8 +32,9 @@ export const getRootLevelTabsComponent = (dashboardLayout: DashboardLayout) => { export const shouldFocusTabs = ( event: { target: { className: string } }, - container: { contains: (arg0: any) => any }, -) => + container: { contains: (arg0: any) => any } | null, + _menuRef: HTMLDivElement | null, +): boolean => // don't focus the tabs when we click on a tab event.target.className === 'ant-tabs-nav-wrap' || - container.contains(event.target); + (container?.contains(event.target) ?? false); diff --git a/superset-frontend/src/dashboard/components/DashboardGrid.tsx b/superset-frontend/src/dashboard/components/DashboardGrid.tsx index a85815c39bc..57159037692 100644 --- a/superset-frontend/src/dashboard/components/DashboardGrid.tsx +++ b/superset-frontend/src/dashboard/components/DashboardGrid.tsx @@ -16,12 +16,10 @@ * specific language governing permissions and limitations * under the License. */ -import { PureComponent, Fragment } from 'react'; -import { withTheme } from '@emotion/react'; +import { Fragment, useCallback, useRef, useState } from 'react'; import classNames from 'classnames'; import { addAlpha } from '@superset-ui/core'; -import { css, styled, type SupersetTheme } from '@apache-superset/core/theme'; -import { t } from '@apache-superset/core/translation'; +import { css, styled, t, useTheme } from '@apache-superset/core/ui'; import { EmptyState } from '@superset-ui/core/components'; import { Icons } from '@superset-ui/core/components/Icons'; import { navigateTo } from 'src/utils/navigationUtils'; @@ -48,11 +46,6 @@ export interface DashboardGridProps { setEditMode?: (editMode: boolean) => void; width: number; dashboardId?: number; - theme: SupersetTheme; -} - -interface DashboardGridState { - isResizing: boolean; } interface DropProps { @@ -131,261 +124,235 @@ const GridColumnGuide = styled.div` `}; `; -class DashboardGrid extends PureComponent< - DashboardGridProps, - DashboardGridState -> { - grid: HTMLDivElement | null; +function DashboardGrid({ + depth, + editMode, + canEdit, + gridComponent, + handleComponentDrop, + isComponentVisible, + resizeComponent, + setDirectPathToChild, + setEditMode, + width, + dashboardId, +}: DashboardGridProps) { + const theme = useTheme(); + const [isResizing, setIsResizing] = useState(false); + const gridRef = useRef(null); - constructor(props: DashboardGridProps) { - super(props); - this.state = { - isResizing: false, - }; - this.grid = null; - this.handleResizeStart = this.handleResizeStart.bind(this); - this.handleResize = this.handleResize.bind(this); - this.handleResizeStop = this.handleResizeStop.bind(this); - this.handleTopDropTargetDrop = this.handleTopDropTargetDrop.bind(this); - this.getRowGuidePosition = this.getRowGuidePosition.bind(this); - this.setGridRef = this.setGridRef.bind(this); - this.handleChangeTab = this.handleChangeTab.bind(this); - } + const setGridRef = useCallback((ref: HTMLDivElement | null): void => { + gridRef.current = ref; + }, []); - getRowGuidePosition(resizeRef: HTMLElement | null): number | null { - if (resizeRef && this.grid) { - return ( - resizeRef.getBoundingClientRect().bottom - - this.grid.getBoundingClientRect().top - - 2 - ); - } - return null; - } + const handleResizeStart = useCallback((): void => { + setIsResizing(true); + }, []); - setGridRef(ref: HTMLDivElement | null): void { - this.grid = ref; - } + const handleResize = useCallback( + ( + _event: MouseEvent | TouchEvent, + _direction: string, + _elementRef: HTMLElement, + _delta: { width: number; height: number }, + ): void => { + // no-op: resize position tracking not implemented + }, + [], + ); - handleResizeStart(): void { - this.setState(() => ({ - isResizing: true, - })); - } - - handleResize( - _event: MouseEvent | TouchEvent, - _direction: string, - _elementRef: HTMLElement, - _delta: { width: number; height: number }, - ): void { - // no-op: resize position is tracked via getRowGuidePosition - } - - handleResizeStop( - _event: MouseEvent | TouchEvent, - _direction: string, - _elementRef: HTMLElement, - delta: { width: number; height: number }, - id: string, - ): void { - this.props.resizeComponent({ - id, - width: delta.width, - height: delta.height, - }); - - this.setState(() => ({ - isResizing: false, - })); - } - - handleTopDropTargetDrop(dropResult: DropResult): void { - if (dropResult?.destination) { - this.props.handleComponentDrop({ - ...dropResult, - destination: { - ...dropResult.destination, - // force appending as the first child if top drop target - index: 0, - }, + const handleResizeStop = useCallback( + ( + _event: MouseEvent | TouchEvent, + _direction: string, + _elementRef: HTMLElement, + delta: { width: number; height: number }, + id: string, + ): void => { + resizeComponent({ + id, + width: delta.width, + height: delta.height, }); - } - } - handleChangeTab({ pathToTabIndex }: { pathToTabIndex: string[] }): void { - this.props.setDirectPathToChild(pathToTabIndex); - } + setIsResizing(false); + }, + [resizeComponent], + ); - render() { - const { - gridComponent, - handleComponentDrop, - depth, - width, - isComponentVisible, - editMode, - canEdit, - setEditMode, - dashboardId, - theme, - } = this.props; - const columnPlusGutterWidth = - (width + GRID_GUTTER_SIZE) / GRID_COLUMN_COUNT; + const handleTopDropTargetDrop = useCallback( + (dropResult: DropResult): void => { + if (dropResult?.destination) { + handleComponentDrop({ + ...dropResult, + destination: { + ...dropResult.destination, + // force appending as the first child if top drop target + index: 0, + }, + }); + } + }, + [handleComponentDrop], + ); - const columnWidth = columnPlusGutterWidth - GRID_GUTTER_SIZE; - const { isResizing } = this.state; + const handleChangeTab = useCallback( + ({ pathToTabIndex }: { pathToTabIndex: string[] }): void => { + setDirectPathToChild(pathToTabIndex); + }, + [setDirectPathToChild], + ); - const shouldDisplayEmptyState = gridComponent?.children?.length === 0; - const shouldDisplayTopLevelTabEmptyState = - shouldDisplayEmptyState && gridComponent?.type === TAB_TYPE; + const columnPlusGutterWidth = (width + GRID_GUTTER_SIZE) / GRID_COLUMN_COUNT; - const dashboardEmptyState = editMode && ( - - - {t('Create a new chart')} - - } - buttonAction={() => { - navigateTo(`/chart/add?dashboard_id=${dashboardId}`, { - newWindow: true, - }); - }} - image="chart.svg" - /> - ); + const columnWidth = columnPlusGutterWidth - GRID_GUTTER_SIZE; - const topLevelTabEmptyState = editMode ? ( - - - {t('Create a new chart')} - - } - buttonAction={() => { - navigateTo(`/chart/add?dashboard_id=${dashboardId}`, { - newWindow: true, - }); - }} - image="chart.svg" - /> - ) : ( - { - setEditMode?.(true); - } - : undefined - } - image="chart.svg" - /> - ); + const shouldDisplayEmptyState = gridComponent?.children?.length === 0; + const shouldDisplayTopLevelTabEmptyState = + shouldDisplayEmptyState && gridComponent?.type === TAB_TYPE; - return width < 100 ? null : ( - <> - {shouldDisplayEmptyState && ( - - {shouldDisplayTopLevelTabEmptyState - ? topLevelTabEmptyState - : dashboardEmptyState} - - )} -
- - {/* make the area above components droppable */} - {editMode && ( - - {renderDraggableContent} - - )} - {gridComponent?.children?.map((id, index) => ( - - + + {t('Create a new chart')} + + } + buttonAction={() => { + navigateTo(`/chart/add?dashboard_id=${dashboardId}`, { + newWindow: true, + }); + }} + image="chart.svg" + /> + ); + + const topLevelTabEmptyState = editMode ? ( + + + {t('Create a new chart')} + + } + buttonAction={() => { + navigateTo(`/chart/add?dashboard_id=${dashboardId}`, { + newWindow: true, + }); + }} + image="chart.svg" + /> + ) : ( + { + setEditMode?.(true); + } + : undefined + } + image="chart.svg" + /> + ); + + return width < 100 ? null : ( + <> + {shouldDisplayEmptyState && ( + + {shouldDisplayTopLevelTabEmptyState + ? topLevelTabEmptyState + : dashboardEmptyState} + + )} +
+ + {/* make the area above components droppable */} + {editMode && ( + + {renderDraggableContent} + + )} + {gridComponent?.children?.map((id, index) => ( + + + {/* make the area below components droppable */} + {editMode && ( + + {renderDraggableContent} + + )} + + ))} + {isResizing && + Array(GRID_COLUMN_COUNT) + .fill(null) + .map((_, i) => ( + - {/* make the area below components droppable */} - {editMode && ( - - {renderDraggableContent} - - )} - - ))} - {isResizing && - Array(GRID_COLUMN_COUNT) - .fill(null) - .map((_, i) => ( - - ))} - -
- - ); - } + ))} +
+
+ + ); } -export default withTheme(DashboardGrid); +export default DashboardGrid; diff --git a/superset-frontend/src/dashboard/components/PublishedStatus/index.tsx b/superset-frontend/src/dashboard/components/PublishedStatus/index.tsx index 8696fbbbb67..dce12de6cbd 100644 --- a/superset-frontend/src/dashboard/components/PublishedStatus/index.tsx +++ b/superset-frontend/src/dashboard/components/PublishedStatus/index.tsx @@ -16,8 +16,8 @@ * specific language governing permissions and limitations * under the License. */ -import { Component } from 'react'; -import { t } from '@apache-superset/core/translation'; +import { useCallback } from 'react'; +import { t } from '@apache-superset/core'; import { Tooltip, PublishedLabel } from '@superset-ui/core/components'; import { HeaderProps, HeaderDropdownProps } from '../Header/types'; @@ -43,70 +43,64 @@ const publishedTooltip = t( 'This dashboard is published. Click to make it a draft.', ); -export default class PublishedStatus extends Component { - constructor(props: DashboardPublishedStatusType) { - super(props); - this.togglePublished = this.togglePublished.bind(this); - } +export default function PublishedStatus({ + dashboardId, + userCanEdit, + userCanSave, + isPublished, + savePublished, +}: DashboardPublishedStatusType) { + const togglePublished = useCallback(() => { + savePublished(dashboardId, !isPublished); + }, [dashboardId, isPublished, savePublished]); - togglePublished() { - this.props.savePublished(this.props.dashboardId, !this.props.isPublished); - } - - render() { - const { isPublished, userCanEdit, userCanSave } = this.props; - - // Show everybody the draft badge - if (!isPublished) { - // if they can edit the dash, make the badge a button - if (userCanEdit && userCanSave) { - return ( - -
- -
-
- ); - } + // Show everybody the draft badge + if (!isPublished) { + // if they can edit the dash, make the badge a button + if (userCanEdit && userCanSave) { return ( -
- -
-
- ); - } - - // Show the published badge for the owner of the dashboard to toggle - if (userCanEdit && userCanSave) { - return ( -
); } - - // Don't show anything if one doesn't own the dashboard and it is published - return null; + return ( + +
+ +
+
+ ); } + + // Show the published badge for the owner of the dashboard to toggle + if (userCanEdit && userCanSave) { + return ( + +
+ +
+
+ ); + } + + // Don't show anything if one doesn't own the dashboard and it is published + return null; } diff --git a/superset-frontend/src/dashboard/components/SliceAdder.tsx b/superset-frontend/src/dashboard/components/SliceAdder.tsx index 936badc9d0b..3ba94aaa64d 100644 --- a/superset-frontend/src/dashboard/components/SliceAdder.tsx +++ b/superset-frontend/src/dashboard/components/SliceAdder.tsx @@ -17,13 +17,13 @@ * under the License. */ /* eslint-env browser */ -import { Component } from 'react'; +import { useState, useEffect, useCallback, useRef, useMemo } from 'react'; import AutoSizer from 'react-virtualized-auto-sizer'; import { FixedSizeList as List } from 'react-window'; // @ts-expect-error import { createFilter } from 'react-search-input'; -import { t } from '@apache-superset/core/translation'; -import { styled, css } from '@apache-superset/core/theme'; +import { t } from '@apache-superset/core'; +import { styled, css, useTheme } from '@apache-superset/core/ui'; import { Button, Checkbox, @@ -49,7 +49,6 @@ import { import { debounce, pickBy } from 'lodash'; import { Dispatch } from 'redux'; import { Slice } from 'src/dashboard/types'; -import { withTheme, Theme } from '@emotion/react'; import { navigateTo } from 'src/utils/navigationUtils'; import type { ConnectDragSource } from 'react-dnd'; import AddSliceCard from './AddSliceCard'; @@ -58,7 +57,6 @@ import { DragDroppable } from './dnd/DragDroppable'; import { datasetLabelLower } from 'src/features/semanticLayers/label'; export type SliceAdderProps = { - theme: Theme; fetchSlices: ( userId?: number, filter_value?: string, @@ -77,14 +75,6 @@ export type SliceAdderProps = { dashboardId: number; }; -type SliceAdderState = { - filteredSlices: Slice[]; - searchTerm: string; - sortBy: keyof Slice; - selectedSliceIdsSet: Set; - showOnlyMyCharts: boolean; -}; - const KEYS_TO_FILTERS = ['slice_name', 'viz_type', 'datasource_name']; const KEYS_TO_SORT = { slice_name: t('name'), @@ -174,295 +164,277 @@ function getFilteredSortedSlices( .filter(createFilter(searchTerm, KEYS_TO_FILTERS)) .sort(sortByComparator(sortBy)); } -class SliceAdder extends Component { - private slicesRequest?: AbortController | Promise; - static defaultProps = { - selectedSliceIds: [], - editMode: false, - errorMessage: '', - }; +function SliceAdder({ + fetchSlices, + updateSlices, + isLoading, + slices, + errorMessage = '', + userId, + selectedSliceIds = [], + editMode = false, + dashboardId, +}: SliceAdderProps) { + const theme = useTheme(); + const slicesRequestRef = useRef>(); - constructor(props: SliceAdderProps) { - super(props); - this.state = { - filteredSlices: [], - searchTerm: '', - sortBy: DEFAULT_SORT_KEY, - selectedSliceIdsSet: new Set(props.selectedSliceIds), - showOnlyMyCharts: getItem( - LocalStorageKeys.DashboardEditorShowOnlyMyCharts, - true, - ), - }; - this.rowRenderer = this.rowRenderer.bind(this); - this.searchUpdated = this.searchUpdated.bind(this); - this.handleSelect = this.handleSelect.bind(this); - this.userIdForFetch = this.userIdForFetch.bind(this); - this.onShowOnlyMyCharts = this.onShowOnlyMyCharts.bind(this); - } + const [searchTerm, setSearchTerm] = useState(''); + const [sortBy, setSortBy] = useState(DEFAULT_SORT_KEY); + const [selectedSliceIdsSet, setSelectedSliceIdsSet] = useState( + () => new Set(selectedSliceIds), + ); - userIdForFetch() { - return this.state.showOnlyMyCharts ? this.props.userId : undefined; - } + // Refs to track latest values for cleanup effect + const latestSlicesRef = useRef(slices); + const latestSelectedSliceIdsSetRef = useRef(selectedSliceIdsSet); + const [showOnlyMyCharts, setShowOnlyMyCharts] = useState(() => + getItem(LocalStorageKeys.DashboardEditorShowOnlyMyCharts, true), + ); - componentDidMount() { - this.slicesRequest = this.props.fetchSlices( - this.userIdForFetch(), - '', - this.state.sortBy, - ); - } + // Keep refs updated with latest values + useEffect(() => { + latestSlicesRef.current = slices; + }, [slices]); - componentDidUpdate(prevProps: SliceAdderProps) { - const nextState: SliceAdderState = {} as SliceAdderState; - if (this.props.lastUpdated !== prevProps.lastUpdated) { - nextState.filteredSlices = getFilteredSortedSlices( - this.props.slices, - this.state.searchTerm, - this.state.sortBy, - this.state.showOnlyMyCharts, - this.props.userId, - ); - } + useEffect(() => { + latestSelectedSliceIdsSetRef.current = selectedSliceIdsSet; + }, [selectedSliceIdsSet]); - if (prevProps.selectedSliceIds !== this.props.selectedSliceIds) { - nextState.selectedSliceIdsSet = new Set(this.props.selectedSliceIds); - } - - if (Object.keys(nextState).length) { - this.setState(nextState); - } - } - - componentWillUnmount() { - // Clears the redux store keeping only selected items - const selectedSlices = pickBy(this.props.slices, (value: Slice) => - this.state.selectedSliceIdsSet.has(value.slice_id), - ); - - this.props.updateSlices(selectedSlices); - if (this.slicesRequest instanceof AbortController) { - this.slicesRequest.abort(); - } - } - - handleChange = debounce(value => { - this.searchUpdated(value); - this.slicesRequest = this.props.fetchSlices( - this.userIdForFetch(), - value, - this.state.sortBy, - ); - }, 300); - - searchUpdated(searchTerm: string) { - this.setState(prevState => ({ - searchTerm, - filteredSlices: getFilteredSortedSlices( - this.props.slices, + const filteredSlices = useMemo( + () => + getFilteredSortedSlices( + slices, searchTerm, - prevState.sortBy, - prevState.showOnlyMyCharts, - this.props.userId, - ), - })); - } - - handleSelect(sortBy: keyof Slice) { - this.setState(prevState => ({ - sortBy, - filteredSlices: getFilteredSortedSlices( - this.props.slices, - prevState.searchTerm, sortBy, - prevState.showOnlyMyCharts, - this.props.userId, - ), - })); - this.slicesRequest = this.props.fetchSlices( - this.userIdForFetch(), - this.state.searchTerm, - sortBy, - ); - } - - rowRenderer({ index, style }: { index: number; style: React.CSSProperties }) { - const { filteredSlices, selectedSliceIdsSet } = this.state; - const cellData = filteredSlices[index]; - - const isSelected = selectedSliceIdsSet.has(cellData.slice_id); - const type = CHART_TYPE; - const id = NEW_CHART_ID; - - const meta = { - chartId: cellData.slice_id, - sliceName: cellData.slice_name, - }; - return ( - - {({ dragSourceRef }: { dragSourceRef: ConnectDragSource }) => ( - - )} - - ); - } - - onShowOnlyMyCharts = (showOnlyMyCharts: boolean) => { - if (!showOnlyMyCharts) { - this.slicesRequest = this.props.fetchSlices( - undefined, - this.state.searchTerm, - this.state.sortBy, - ); - } - this.setState(prevState => ({ - showOnlyMyCharts, - filteredSlices: getFilteredSortedSlices( - this.props.slices, - prevState.searchTerm, - prevState.sortBy, showOnlyMyCharts, - this.props.userId, + userId, ), - })); - setItem(LocalStorageKeys.DashboardEditorShowOnlyMyCharts, showOnlyMyCharts); - }; + [slices, searchTerm, sortBy, showOnlyMyCharts, userId], + ); - render() { - const { theme } = this.props; - return ( -
span > :first-of-type { - margin-right: 0; + const userIdForFetch = useCallback( + () => (showOnlyMyCharts ? userId : undefined), + [showOnlyMyCharts, userId], + ); + + // componentDidMount + useEffect(() => { + slicesRequestRef.current = fetchSlices(userIdForFetch(), '', sortBy); + // Only run on mount + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + + // Update selectedSliceIdsSet when selectedSliceIds prop changes + useEffect(() => { + setSelectedSliceIdsSet(new Set(selectedSliceIds)); + }, [selectedSliceIds]); + + // componentWillUnmount + useEffect( + () => () => { + // Clears the redux store keeping only selected items + // Use refs to get latest values on unmount + const selectedSlices = pickBy(latestSlicesRef.current, (value: Slice) => + latestSelectedSliceIdsSetRef.current.has(value.slice_id), + ); + + updateSlices(selectedSlices); + if (slicesRequestRef.current instanceof AbortController) { + slicesRequestRef.current.abort(); + } + }, + [updateSlices], + ); + + const searchUpdated = useCallback((term: string) => { + setSearchTerm(term); + }, []); + + const handleChange = useMemo( + () => + debounce((value: string) => { + searchUpdated(value); + slicesRequestRef.current = fetchSlices(userIdForFetch(), value, sortBy); + }, 300), + [fetchSlices, searchUpdated, sortBy, userIdForFetch], + ); + + const handleSelect = useCallback( + (newSortBy: keyof Slice) => { + setSortBy(newSortBy); + slicesRequestRef.current = fetchSlices( + userIdForFetch(), + searchTerm, + newSortBy, + ); + }, + [fetchSlices, searchTerm, userIdForFetch], + ); + + const onShowOnlyMyCharts = useCallback( + (checked: boolean) => { + if (!checked) { + slicesRequestRef.current = fetchSlices(undefined, searchTerm, sortBy); + } + setShowOnlyMyCharts(checked); + setItem(LocalStorageKeys.DashboardEditorShowOnlyMyCharts, checked); + }, + [fetchSlices, searchTerm, sortBy], + ); + + const rowRenderer = useCallback( + ({ index, style }: { index: number; style: React.CSSProperties }) => { + const cellData = filteredSlices[index]; + + const isSelected = selectedSliceIdsSet.has(cellData.slice_id); + const type = CHART_TYPE; + const id = NEW_CHART_ID; + + const meta = { + chartId: cellData.slice_id, + sliceName: cellData.slice_name, + }; + return ( + + {({ dragSourceRef }: { dragSourceRef: ConnectDragSource }) => ( + + )} + + ); + }, + [filteredSlices, selectedSliceIdsSet, editMode], + ); + + return ( +
span > :first-of-type { + margin-right: 0; + } + `} + > + + } + onClick={() => + navigateTo(`/chart/add?dashboard_id=${dashboardId}`, { + newWindow: true, + }) + } + > + {t('Create new chart')} + + + + handleChange(ev.target.value)} + data-test="dashboard-charts-filter-search-input" + /> + ({ + label: t('Sort by %s', label), + value: key, + }))} + placeholder={t('Sort by')} + /> + +
css` + display: flex; + flex-direction: row; + justify-content: flex-start; + align-items: center; + gap: ${themeObj.sizeUnit}px; + padding: 0 ${themeObj.sizeUnit * 3}px ${themeObj.sizeUnit * 4}px + ${themeObj.sizeUnit * 3}px; `} > - - - } - onClick={() => - navigateTo(`/chart/add?dashboard_id=${this.props.dashboardId}`, { - newWindow: true, - }) - } - > - {t('Create new chart')} - - - - this.handleChange(ev.target.value)} - data-test="dashboard-charts-filter-search-input" - /> - ({ - label: t('Sort by %s', label), - value: key, - }))} - placeholder={t('Sort by')} - /> - + onShowOnlyMyCharts(e.target.checked)} + checked={showOnlyMyCharts} + /> + {t('Show only my charts')} + +
+ {isLoading && } + {!isLoading && filteredSlices.length > 0 && ( + + + {({ height, width }: { height: number; width: number }) => ( + filteredSlices[index].slice_id} + > + {rowRenderer} + + )} + + + )} + {errorMessage && (
css` - display: flex; - flex-direction: row; - justify-content: flex-start; - align-items: center; - gap: ${theme.sizeUnit}px; - padding: 0 ${theme.sizeUnit * 3}px ${theme.sizeUnit * 4}px - ${theme.sizeUnit * 3}px; + css={css` + padding: 16px; `} > - this.onShowOnlyMyCharts(e.target.checked)} - checked={this.state.showOnlyMyCharts} - /> - {t('Show only my charts')} - + {errorMessage}
- {this.props.isLoading && } - {!this.props.isLoading && this.state.filteredSlices.length > 0 && ( - - - {({ height, width }: { height: number; width: number }) => ( - this.state.filteredSlices[index].slice_id} - > - {this.rowRenderer} - - )} - - - )} - {this.props.errorMessage && ( -
- {this.props.errorMessage} -
- )} - {/* Drag preview is just a single fixed-position element */} - -
- ); - } + )} + {/* Drag preview is just a single fixed-position element */} + +
+ ); } -export default withTheme(SliceAdder); +export default SliceAdder; diff --git a/superset-frontend/src/dashboard/components/UndoRedoKeyListeners/index.tsx b/superset-frontend/src/dashboard/components/UndoRedoKeyListeners/index.tsx index 89dd62694ed..7417dcdbf63 100644 --- a/superset-frontend/src/dashboard/components/UndoRedoKeyListeners/index.tsx +++ b/superset-frontend/src/dashboard/components/UndoRedoKeyListeners/index.tsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { PureComponent } from 'react'; +import { useCallback, useEffect } from 'react'; import { HeaderProps } from '../Header/types'; type UndoRedoKeyListenersProps = { @@ -24,43 +24,38 @@ type UndoRedoKeyListenersProps = { onRedo: HeaderProps['onRedo']; }; -class UndoRedoKeyListeners extends PureComponent { - constructor(props: UndoRedoKeyListenersProps) { - super(props); - this.handleKeydown = this.handleKeydown.bind(this); - } +function UndoRedoKeyListeners({ onUndo, onRedo }: UndoRedoKeyListenersProps) { + const handleKeydown = useCallback( + (event: KeyboardEvent) => { + const controlOrCommand = event.ctrlKey || event.metaKey; + if (controlOrCommand) { + const isZChar = event.key === 'z' || event.keyCode === 90; + const isYChar = event.key === 'y' || event.keyCode === 89; + const isEditingMarkdown = document?.querySelector( + '.dashboard-markdown--editing', + ); + const isEditingTitle = document?.querySelector( + '.editable-title--editing', + ); - componentDidMount() { - document.addEventListener('keydown', this.handleKeydown); - } - - componentWillUnmount() { - document.removeEventListener('keydown', this.handleKeydown); - } - - handleKeydown(event: KeyboardEvent) { - const controlOrCommand = event.ctrlKey || event.metaKey; - if (controlOrCommand) { - const isZChar = event.key === 'z' || event.keyCode === 90; - const isYChar = event.key === 'y' || event.keyCode === 89; - const isEditingMarkdown = document?.querySelector( - '.dashboard-markdown--editing', - ); - const isEditingTitle = document?.querySelector( - '.editable-title--editing', - ); - - if (!isEditingMarkdown && !isEditingTitle && (isZChar || isYChar)) { - event.preventDefault(); - const func = isZChar ? this.props.onUndo : this.props.onRedo; - func(); + if (!isEditingMarkdown && !isEditingTitle && (isZChar || isYChar)) { + event.preventDefault(); + const func = isZChar ? onUndo : onRedo; + func(); + } } - } - } + }, + [onUndo, onRedo], + ); - render() { - return null; - } + useEffect(() => { + document.addEventListener('keydown', handleKeydown); + return () => { + document.removeEventListener('keydown', handleKeydown); + }; + }, [handleKeydown]); + + return null; } export default UndoRedoKeyListeners; diff --git a/superset-frontend/src/dashboard/components/dnd/DragDroppable.tsx b/superset-frontend/src/dashboard/components/dnd/DragDroppable.tsx index cd603b6af6d..d1daab5115e 100644 --- a/superset-frontend/src/dashboard/components/dnd/DragDroppable.tsx +++ b/superset-frontend/src/dashboard/components/dnd/DragDroppable.tsx @@ -32,7 +32,7 @@ import { ConnectDropTarget, } from 'react-dnd'; import cx from 'classnames'; -import { css, styled } from '@apache-superset/core/theme'; +import { css, styled } from '@apache-superset/core/ui'; import { dragConfig, dropConfig } from './dragDroppableConfig'; import type { DragDroppableProps as BaseDragDroppableProps } from './dragDroppableConfig'; @@ -92,15 +92,6 @@ const DragDroppableStyles = styled.div` &.dragdroppable-row { width: 100%; } - /* workaround to avoid a bug in react-dnd where the drag - preview expands outside of the bounds of the drag source card, see: - https://github.com/react-dnd/react-dnd/issues/832 */ - &.dragdroppable-column { - /* for chrome */ - transform: translate3d(0, 0, 0); - /* for safari */ - backface-visibility: hidden; - } &.dragdroppable-column .resizable-container span div { z-index: 10; @@ -122,15 +113,22 @@ const DragDroppableStyles = styled.div` } `}; `; + +/** + * Note: This component remains a class component because it is tightly integrated + * with react-dnd's class-based HOC system (DragSource/DropTarget). The HOCs + * access component instance properties directly (mounted, ref, props, setState) + * in the hover/drop callbacks defined in dragDroppableConfig.ts. + * + * Converting to a function component would require migrating to react-dnd's + * hooks API (useDrag/useDrop), which would be a more extensive refactor. + */ // export unwrapped component for testing +// eslint-disable-next-line react-prefer-function-component/react-prefer-function-component -- react-dnd class-based HOC requires class component instance properties export class UnwrappedDragDroppable extends PureComponent< DragDroppableAllProps, DragDroppableState > { - mounted: boolean; - - ref: HTMLDivElement | null; - static defaultProps = { className: null, style: null, @@ -152,6 +150,10 @@ export class UnwrappedDragDroppable extends PureComponent< dragPreviewRef() {}, }; + mounted: boolean; + + ref: HTMLDivElement | null; + constructor(props: DragDroppableAllProps) { super(props); this.state = { @@ -283,7 +285,6 @@ export class UnwrappedDragDroppable extends PureComponent< // react-dnd's DragSource/DropTarget HOC types don't play well with // class components using spread config tuples, so we use type assertions here -// eslint-disable-next-line @typescript-eslint/no-explicit-any const DragDroppableAsAny = UnwrappedDragDroppable as unknown as ReactComponentType< Record diff --git a/superset-frontend/src/dashboard/components/filterscope/FilterScopeModal.tsx b/superset-frontend/src/dashboard/components/filterscope/FilterScopeModal.tsx index 5dd6b01ee54..13cedc322a4 100644 --- a/superset-frontend/src/dashboard/components/filterscope/FilterScopeModal.tsx +++ b/superset-frontend/src/dashboard/components/filterscope/FilterScopeModal.tsx @@ -16,8 +16,8 @@ * specific language governing permissions and limitations * under the License. */ -import { createRef, PureComponent } from 'react'; -import { styled } from '@apache-superset/core/theme'; +import { useRef, useCallback } from 'react'; +import { styled } from '@apache-superset/core/ui'; import { ModalTrigger, ModalTriggerRef, @@ -33,39 +33,29 @@ const FilterScopeModalBody = styled.div(({ theme: { sizeUnit } }) => ({ paddingBottom: sizeUnit * 3, })); -export default class FilterScopeModal extends PureComponent< - FilterScopeModalProps, - {} -> { - modal: ModalTriggerRef; +export default function FilterScopeModal({ + triggerNode, +}: FilterScopeModalProps) { + const modalRef = useRef(null); - constructor(props: FilterScopeModalProps) { - super(props); + const handleCloseModal = useCallback((): void => { + modalRef.current?.close?.(); + }, []); - this.modal = createRef() as ModalTriggerRef; - this.handleCloseModal = this.handleCloseModal.bind(this); - } + const filterScopeProps = { + onCloseModal: handleCloseModal, + }; - handleCloseModal(): void { - this?.modal?.current?.close?.(); - } - - render() { - const filterScopeProps = { - onCloseModal: this.handleCloseModal, - }; - - return ( - - - - } - width="80%" - /> - ); - } + return ( + + + + } + width="80%" + /> + ); } diff --git a/superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.test.tsx b/superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.test.tsx new file mode 100644 index 00000000000..f76ad264177 --- /dev/null +++ b/superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.test.tsx @@ -0,0 +1,265 @@ +/** + * 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 { + cleanup, + render, + screen, + userEvent, +} from 'spec/helpers/testing-library'; +import FilterScopeSelector from './FilterScopeSelector'; +import type { DashboardLayout } from 'src/dashboard/types'; + +// --- Mock child components --- + +jest.mock('./FilterFieldTree', () => ({ + __esModule: true, + default: (props: Record) => ( +
+ FilterFieldTree (checked={String(props.checked)}) +
+ ), +})); + +jest.mock('./FilterScopeTree', () => ({ + __esModule: true, + default: (props: Record) => ( +
+ FilterScopeTree (checked={String(props.checked)}) +
+ ), +})); + +// --- Mock utility functions --- + +jest.mock('src/dashboard/util/getFilterFieldNodesTree', () => ({ + __esModule: true, + default: jest.fn(() => [ + { + value: 'ALL_FILTERS_ROOT', + label: 'All filters', + children: [ + { + value: 1, + label: 'Filter A', + children: [ + { value: '1_column_b', label: 'Filter B' }, + { value: '1_column_c', label: 'Filter C' }, + ], + }, + ], + }, + ]), +})); + +jest.mock('src/dashboard/util/getFilterScopeNodesTree', () => ({ + __esModule: true, + default: jest.fn(() => [ + { + value: 'ROOT_ID', + label: 'All charts', + children: [{ value: 2, label: 'Chart A' }], + }, + ]), +})); + +jest.mock('src/dashboard/util/getFilterScopeParentNodes', () => ({ + __esModule: true, + default: jest.fn(() => ['ROOT_ID']), +})); + +jest.mock('src/dashboard/util/buildFilterScopeTreeEntry', () => ({ + __esModule: true, + default: jest.fn(() => ({})), +})); + +jest.mock('src/dashboard/util/getKeyForFilterScopeTree', () => ({ + __esModule: true, + default: jest.fn(() => '1_column_b'), +})); + +jest.mock('src/dashboard/util/getSelectedChartIdForFilterScopeTree', () => ({ + __esModule: true, + default: jest.fn(() => 1), +})); + +jest.mock('src/dashboard/util/getFilterScopeFromNodesTree', () => ({ + __esModule: true, + default: jest.fn(() => ({ scope: ['ROOT_ID'], immune: [] })), +})); + +jest.mock('src/dashboard/util/getRevertedFilterScope', () => ({ + __esModule: true, + default: jest.fn(() => ({})), +})); + +jest.mock('src/dashboard/util/activeDashboardFilters', () => ({ + getChartIdsInFilterScope: jest.fn(() => [2, 3]), +})); + +afterEach(() => { + cleanup(); + jest.clearAllMocks(); +}); + +const mockDashboardFilters = { + 1: { + chartId: 1, + componentId: 'component-1', + filterName: 'Filter A', + datasourceId: 'ds-1', + directPathToFilter: ['ROOT_ID', 'GRID', 'CHART_1'], + isDateFilter: false, + isInstantFilter: false, + columns: { column_b: undefined, column_c: undefined }, + labels: { column_b: 'Filter B', column_c: 'Filter C' }, + scopes: { + column_b: { immune: [], scope: ['ROOT_ID'] }, + column_c: { immune: [], scope: ['ROOT_ID'] }, + }, + }, +}; + +const mockLayout: DashboardLayout = { + ROOT_ID: { children: ['GRID'], id: 'ROOT_ID', type: 'ROOT' }, + GRID: { + children: ['CHART_1', 'CHART_2'], + id: 'GRID', + type: 'GRID', + parents: ['ROOT_ID'], + }, + CHART_1: { + meta: { chartId: 1, sliceName: 'Chart 1' }, + children: [], + id: 'CHART_1', + type: 'CHART', + parents: ['ROOT_ID', 'GRID'], + }, + CHART_2: { + meta: { chartId: 2, sliceName: 'Chart 2' }, + children: [], + id: 'CHART_2', + type: 'CHART', + parents: ['ROOT_ID', 'GRID'], + }, +} as unknown as DashboardLayout; + +const defaultProps = { + dashboardFilters: mockDashboardFilters, + layout: mockLayout, + updateDashboardFiltersScope: jest.fn(), + setUnsavedChanges: jest.fn(), + onCloseModal: jest.fn(), +}; + +test('renders the header, filter field panel, and scope panel', () => { + render(, { useRedux: true }); + + expect(screen.getByText('Configure filter scopes')).toBeInTheDocument(); + expect(screen.getByTestId('filter-field-tree')).toBeInTheDocument(); + expect(screen.getByTestId('filter-scope-tree')).toBeInTheDocument(); +}); + +test('renders the search input with correct placeholder', () => { + render(, { useRedux: true }); + + const searchInput = screen.getByPlaceholderText('Search...'); + expect(searchInput).toBeInTheDocument(); + expect(searchInput).toHaveAttribute('type', 'text'); +}); + +test('renders Close and Save buttons when filters exist', () => { + render(, { useRedux: true }); + + expect(screen.getByRole('button', { name: 'Close' })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Save' })).toBeInTheDocument(); +}); + +test('renders only Close button and a warning when no filters exist', () => { + render(, { + useRedux: true, + }); + + expect( + screen.getByText('There are no filters in this dashboard.'), + ).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Close' })).toBeInTheDocument(); + expect( + screen.queryByRole('button', { name: 'Save' }), + ).not.toBeInTheDocument(); +}); + +test('does not render FilterFieldTree or FilterScopeTree when no filters exist', () => { + render(, { + useRedux: true, + }); + + expect(screen.queryByTestId('filter-field-tree')).not.toBeInTheDocument(); + expect(screen.queryByTestId('filter-scope-tree')).not.toBeInTheDocument(); +}); + +test('calls onCloseModal when Close button is clicked', () => { + const onCloseModal = jest.fn(); + render( + , + { useRedux: true }, + ); + + userEvent.click(screen.getByRole('button', { name: 'Close' })); + + expect(onCloseModal).toHaveBeenCalledTimes(1); +}); + +test('calls updateDashboardFiltersScope, setUnsavedChanges, and onCloseModal when Save is clicked', () => { + const updateDashboardFiltersScope = jest.fn(); + const setUnsavedChanges = jest.fn(); + const onCloseModal = jest.fn(); + + render( + , + { useRedux: true }, + ); + + userEvent.click(screen.getByRole('button', { name: 'Save' })); + + expect(updateDashboardFiltersScope).toHaveBeenCalledTimes(1); + expect(setUnsavedChanges).toHaveBeenCalledWith(true); + expect(onCloseModal).toHaveBeenCalledTimes(1); +}); + +test('renders the editing filters name section with "Editing 1 filter:" label', () => { + render(, { useRedux: true }); + + expect(screen.getByText('Editing 1 filter:')).toBeInTheDocument(); + // The active filter label should appear (column_b maps to "Filter B") + expect(screen.getByText('Filter B')).toBeInTheDocument(); +}); + +test('updates search text when typing in the search input', () => { + render(, { useRedux: true }); + + const searchInput = screen.getByPlaceholderText('Search...'); + userEvent.type(searchInput, 'Chart'); + + expect(searchInput).toHaveValue('Chart'); +}); diff --git a/superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.tsx b/superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.tsx index 63e585d852f..562dc03038a 100644 --- a/superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.tsx +++ b/superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.tsx @@ -16,11 +16,16 @@ * specific language governing permissions and limitations * under the License. */ -import { PureComponent, ChangeEvent, type ReactElement } from 'react'; +import { + useState, + useCallback, + useMemo, + ChangeEvent, + type ReactElement, +} from 'react'; import cx from 'classnames'; import { Button, Input } from '@superset-ui/core/components'; -import { css, styled } from '@apache-superset/core/theme'; -import { t } from '@apache-superset/core/translation'; +import { css, styled, t } from '@apache-superset/core/ui'; import buildFilterScopeTreeEntry from 'src/dashboard/util/buildFilterScopeTreeEntry'; import getFilterScopeNodesTree from 'src/dashboard/util/getFilterScopeNodesTree'; @@ -90,30 +95,6 @@ export interface FilterScopeSelectorProps { onCloseModal: () => void; } -interface FilterScopeSelectorStateWithSelector { - showSelector: true; - activeFilterField: string | null; - searchText: string; - filterScopeMap: FilterScopeMap; - filterFieldNodes: FilterFieldNode[]; - checkedFilterFields: string[]; - expandedFilterIds: (string | number)[]; -} - -interface FilterScopeSelectorStateWithoutSelector { - showSelector: false; - activeFilterField?: undefined; - searchText?: undefined; - filterScopeMap?: undefined; - filterFieldNodes?: undefined; - checkedFilterFields?: undefined; - expandedFilterIds?: undefined; -} - -type FilterScopeSelectorState = - | FilterScopeSelectorStateWithSelector - | FilterScopeSelectorStateWithoutSelector; - const ScopeContainer = styled.div` ${({ theme }) => css` display: flex; @@ -389,271 +370,358 @@ const ActionsContainer = styled.div` `} `; -export default class FilterScopeSelector extends PureComponent< - FilterScopeSelectorProps, - FilterScopeSelectorState -> { - allfilterFields: string[]; +function initializeState( + dashboardFilters: Record, + layout: DashboardLayout, +) { + if (Object.keys(dashboardFilters).length === 0) { + return { + showSelector: false as const, + allFilterFields: [] as string[], + defaultFilterKey: '', + }; + } - defaultFilterKey: string; + // display filter fields in tree structure + const filterFieldNodes = getFilterFieldNodesTree({ + dashboardFilters, + }); + // filterFieldNodes root node is dashboard_root component, + // so that we can offer a select/deselect all link + const filtersNodes = filterFieldNodes[0].children ?? []; + const allFilterFields: string[] = []; + filtersNodes.forEach(({ children }) => { + (children ?? []).forEach(child => { + allFilterFields.push(String(child.value)); + }); + }); + const defaultFilterKey = String(filtersNodes[0]?.children?.[0]?.value ?? ''); - constructor(props: FilterScopeSelectorProps) { - super(props); - - this.allfilterFields = []; - this.defaultFilterKey = ''; - - const { dashboardFilters, layout } = props; - - if (Object.keys(dashboardFilters).length > 0) { - // display filter fields in tree structure - const filterFieldNodes = getFilterFieldNodesTree({ - dashboardFilters, - }); - // filterFieldNodes root node is dashboard_root component, - // so that we can offer a select/deselect all link - const filtersNodes = filterFieldNodes[0].children ?? []; - this.allfilterFields = []; - filtersNodes.forEach(({ children }) => { - (children ?? []).forEach(child => { - this.allfilterFields.push(String(child.value)); + // build FilterScopeTree object for each filterKey + const filterScopeMap: FilterScopeMap = Object.values( + dashboardFilters, + ).reduce((map, { chartId: filterId, columns }) => { + const filterScopeByChartId = Object.keys(columns).reduce( + (mapByChartId, columnName) => { + const filterKey = getDashboardFilterKey({ + chartId: String(filterId), + column: columnName, }); - }); - this.defaultFilterKey = String( - filtersNodes[0]?.children?.[0]?.value ?? '', - ); - - // build FilterScopeTree object for each filterKey - const filterScopeMap: FilterScopeMap = Object.values( - dashboardFilters, - ).reduce((map, { chartId: filterId, columns }) => { - const filterScopeByChartId = Object.keys( - columns, - ).reduce((mapByChartId, columnName) => { - const filterKey = getDashboardFilterKey({ - chartId: String(filterId), - column: columnName, - }); - const nodes = getFilterScopeNodesTree({ - components: layout, - filterFields: [filterKey], - selectedChartId: filterId, - }); - const expanded = getFilterScopeParentNodes(nodes, 1); - const chartIdsInFilterScope = ( - getChartIdsInFilterScope({ - filterScope: dashboardFilters[filterId].scopes[columnName], - }) || [] - ).filter((id: number) => id !== filterId); - - return { - ...mapByChartId, - [filterKey]: { - // unfiltered nodes - nodes, - // filtered nodes in display if searchText is not empty - nodesFiltered: [...nodes], - checked: chartIdsInFilterScope, - expanded, - }, - }; - }, {}); + const nodes = getFilterScopeNodesTree({ + components: layout, + filterFields: [filterKey], + selectedChartId: filterId, + }); + const expanded = getFilterScopeParentNodes(nodes, 1); + const chartIdsInFilterScope = ( + getChartIdsInFilterScope({ + filterScope: dashboardFilters[filterId].scopes[columnName], + }) || [] + ).filter((id: number) => id !== filterId); return { - ...map, - ...filterScopeByChartId, + ...mapByChartId, + [filterKey]: { + // unfiltered nodes + nodes, + // filtered nodes in display if searchText is not empty + nodesFiltered: [...nodes], + checked: chartIdsInFilterScope, + expanded, + }, }; - }, {}); - - // initial state: active defaultFilerKey - const { chartId } = getChartIdAndColumnFromFilterKey( - this.defaultFilterKey, - ); - const checkedFilterFields: string[] = []; - const activeFilterField = this.defaultFilterKey; - // expand defaultFilterKey in filter field tree - const expandedFilterIds: (string | number)[] = [ - ALL_FILTERS_ROOT, - chartId, - ]; - - const filterScopeTreeEntry = buildFilterScopeTreeEntry({ - checkedFilterFields, - activeFilterField, - filterScopeMap, - layout, - }); - this.state = { - showSelector: true, - activeFilterField, - searchText: '', - filterScopeMap: { - ...filterScopeMap, - ...filterScopeTreeEntry, - } as FilterScopeMap, - filterFieldNodes, - checkedFilterFields, - expandedFilterIds, - }; - } else { - this.state = { - showSelector: false, - }; - } - - this.filterNodes = this.filterNodes.bind(this); - this.onChangeFilterField = this.onChangeFilterField.bind(this); - this.onCheckFilterScope = this.onCheckFilterScope.bind(this); - this.onExpandFilterScope = this.onExpandFilterScope.bind(this); - this.onSearchInputChange = this.onSearchInputChange.bind(this); - this.onCheckFilterField = this.onCheckFilterField.bind(this); - this.onExpandFilterField = this.onExpandFilterField.bind(this); - this.onClose = this.onClose.bind(this); - this.onSave = this.onSave.bind(this); - } - - onCheckFilterScope(checked: (string | number)[] = []): void { - const state = this.state as FilterScopeSelectorStateWithSelector; - const { activeFilterField, filterScopeMap, checkedFilterFields } = state; - - const key = getKeyForFilterScopeTree({ - activeFilterField: activeFilterField ?? undefined, - checkedFilterFields, - }); - const editingList = activeFilterField - ? [activeFilterField] - : checkedFilterFields; - const updatedEntry = { - ...filterScopeMap[key], - checked, - }; - - const updatedFilterScopeMap = getRevertedFilterScope({ - checked, - filterFields: editingList, - filterScopeMap, - }); - - this.setState(() => ({ - filterScopeMap: { - ...filterScopeMap, - ...updatedFilterScopeMap, - [key]: updatedEntry, - } as FilterScopeMap, - })); - } - - onExpandFilterScope(expanded: string[] = []): void { - const state = this.state as FilterScopeSelectorStateWithSelector; - const { activeFilterField, checkedFilterFields, filterScopeMap } = state; - const key = getKeyForFilterScopeTree({ - activeFilterField: activeFilterField ?? undefined, - checkedFilterFields, - }); - const updatedEntry = { - ...filterScopeMap[key], - expanded, - }; - this.setState(() => ({ - filterScopeMap: { - ...filterScopeMap, - [key]: updatedEntry, }, - })); - } + {}, + ); - onCheckFilterField(checkedFilterFields: string[] = []): void { - const { layout } = this.props; - const state = this.state as FilterScopeSelectorStateWithSelector; - const { filterScopeMap } = state; - const filterScopeTreeEntry = buildFilterScopeTreeEntry({ - checkedFilterFields, - activeFilterField: undefined, - filterScopeMap, - layout, - }); + return { + ...map, + ...filterScopeByChartId, + }; + }, {}); - this.setState(() => ({ - activeFilterField: null, - checkedFilterFields, + // initial state: active defaultFilerKey + const { chartId } = getChartIdAndColumnFromFilterKey(defaultFilterKey); + const checkedFilterFields: string[] = []; + const activeFilterField = defaultFilterKey; + // expand defaultFilterKey in filter field tree + const expandedFilterIds: (string | number)[] = [ALL_FILTERS_ROOT, chartId]; + + const filterScopeTreeEntry = buildFilterScopeTreeEntry({ + checkedFilterFields, + activeFilterField, + filterScopeMap, + layout, + }); + + return { + showSelector: true as const, + allFilterFields, + defaultFilterKey, + initialState: { + activeFilterField, + searchText: '', filterScopeMap: { ...filterScopeMap, ...filterScopeTreeEntry, - }, - })); - } - - onExpandFilterField(expandedFilterIds: (string | number)[] = []): void { - this.setState(() => ({ - expandedFilterIds, - })); - } - - onChangeFilterField(filterField: { value?: string } = {}): void { - const { layout } = this.props; - const nextActiveFilterField = filterField.value; - const state = this.state as FilterScopeSelectorStateWithSelector; - const { - activeFilterField: currentActiveFilterField, + } as FilterScopeMap, + filterFieldNodes, checkedFilterFields, - filterScopeMap, - } = state; + expandedFilterIds, + }, + }; +} - // we allow single edit and multiple edit in the same view. - // if user click on the single filter field, - // will show filter scope for the single field. - // if user click on the same filter filed again, - // will toggle off the single filter field, - // and allow multi-edit all checked filter fields. - if (nextActiveFilterField === currentActiveFilterField) { - const filterScopeTreeEntry = buildFilterScopeTreeEntry({ +export default function FilterScopeSelector({ + dashboardFilters, + layout, + updateDashboardFiltersScope, + setUnsavedChanges, + onCloseModal, +}: FilterScopeSelectorProps): ReactElement { + const initialized = useMemo( + () => initializeState(dashboardFilters, layout), + // Only initialize once on mount + // eslint-disable-next-line react-hooks/exhaustive-deps + [], + ); + + const { showSelector, allFilterFields } = initialized; + + const [activeFilterField, setActiveFilterField] = useState( + () => + initialized.showSelector + ? initialized.initialState.activeFilterField + : null, + ); + const [searchText, setSearchText] = useState(() => + initialized.showSelector ? initialized.initialState.searchText : '', + ); + const [filterScopeMap, setFilterScopeMap] = useState(() => + initialized.showSelector ? initialized.initialState.filterScopeMap : {}, + ); + const [filterFieldNodes] = useState(() => + initialized.showSelector ? initialized.initialState.filterFieldNodes : [], + ); + const [checkedFilterFields, setCheckedFilterFields] = useState( + () => + initialized.showSelector + ? initialized.initialState.checkedFilterFields + : [], + ); + const [expandedFilterIds, setExpandedFilterIds] = useState< + (string | number)[] + >(() => + initialized.showSelector ? initialized.initialState.expandedFilterIds : [], + ); + + const filterNodes = useCallback( + ( + filtered: FilterScopeTreeNode[] = [], + node: FilterScopeTreeNode = { value: '', label: '' }, + currentSearchText: string, + ): FilterScopeTreeNode[] => { + const filterNodesRecursive = ( + f: FilterScopeTreeNode[], + n: FilterScopeTreeNode, + ): FilterScopeTreeNode[] => filterNodes(f, n, currentSearchText); + + const children = (node.children || []).reduce( + filterNodesRecursive, + [], + ); + + if ( + // Node's label matches the search string + node.label + .toLocaleLowerCase() + .indexOf((currentSearchText ?? '').toLocaleLowerCase()) > -1 || + // Or a children has a matching node + children.length + ) { + filtered.push({ ...node, children }); + } + + return filtered; + }, + [], + ); + + const filterTree = useCallback( + (currentSearchText: string) => { + const key = getKeyForFilterScopeTree({ + activeFilterField: activeFilterField ?? undefined, checkedFilterFields, + }); + + // Reset nodes back to unfiltered state + if (!currentSearchText) { + setFilterScopeMap(prev => ({ + ...prev, + [key]: { + ...prev[key], + nodesFiltered: prev[key].nodes, + }, + })); + } else { + setFilterScopeMap(prev => { + const nodesFiltered = prev[key].nodes.reduce( + (filtered, node) => filterNodes(filtered, node, currentSearchText), + [], + ); + const expanded = getFilterScopeParentNodes([...nodesFiltered]); + + return { + ...prev, + [key]: { + ...prev[key], + nodesFiltered, + expanded, + }, + }; + }); + } + }, + [activeFilterField, checkedFilterFields, filterNodes], + ); + + const onCheckFilterScope = useCallback( + (checked: (string | number)[] = []): void => { + const key = getKeyForFilterScopeTree({ + activeFilterField: activeFilterField ?? undefined, + checkedFilterFields, + }); + const editingList = activeFilterField + ? [activeFilterField] + : checkedFilterFields; + + const updatedFilterScopeMap = getRevertedFilterScope({ + checked, + filterFields: editingList, + filterScopeMap, + }); + + setFilterScopeMap({ + ...filterScopeMap, + ...updatedFilterScopeMap, + [key]: { + ...filterScopeMap[key], + checked, + }, + } as FilterScopeMap); + }, + [activeFilterField, checkedFilterFields, filterScopeMap], + ); + + const onExpandFilterScope = useCallback( + (expanded: string[] = []): void => { + const key = getKeyForFilterScopeTree({ + activeFilterField: activeFilterField ?? undefined, + checkedFilterFields, + }); + + setFilterScopeMap(prev => ({ + ...prev, + [key]: { + ...prev[key], + expanded, + }, + })); + }, + [activeFilterField, checkedFilterFields], + ); + + const onCheckFilterField = useCallback( + (newCheckedFilterFields: string[] = []): void => { + const filterScopeTreeEntry = buildFilterScopeTreeEntry({ + checkedFilterFields: newCheckedFilterFields, activeFilterField: undefined, filterScopeMap, layout, }); - this.setState({ - activeFilterField: null, - filterScopeMap: { + setActiveFilterField(null); + setCheckedFilterFields(newCheckedFilterFields); + setFilterScopeMap({ + ...filterScopeMap, + ...filterScopeTreeEntry, + }); + }, + [filterScopeMap, layout], + ); + + const onExpandFilterField = useCallback( + (newExpandedFilterIds: (string | number)[] = []): void => { + setExpandedFilterIds(newExpandedFilterIds); + }, + [], + ); + + const onChangeFilterField = useCallback( + (filterField: { value?: string } = {}): void => { + const nextActiveFilterField = filterField.value; + + // we allow single edit and multiple edit in the same view. + // if user click on the single filter field, + // will show filter scope for the single field. + // if user click on the same filter filed again, + // will toggle off the single filter field, + // and allow multi-edit all checked filter fields. + if (nextActiveFilterField === activeFilterField) { + const filterScopeTreeEntry = buildFilterScopeTreeEntry({ + checkedFilterFields, + activeFilterField: undefined, + filterScopeMap, + layout, + }); + + setActiveFilterField(null); + setFilterScopeMap({ ...filterScopeMap, ...filterScopeTreeEntry, - } as FilterScopeMap, - }); - } else if ( - nextActiveFilterField && - this.allfilterFields.includes(nextActiveFilterField) - ) { - const filterScopeTreeEntry = buildFilterScopeTreeEntry({ - checkedFilterFields, - activeFilterField: nextActiveFilterField, - filterScopeMap, - layout, - }); + }); + } else if ( + nextActiveFilterField && + allFilterFields.includes(nextActiveFilterField) + ) { + const filterScopeTreeEntry = buildFilterScopeTreeEntry({ + checkedFilterFields, + activeFilterField: nextActiveFilterField, + filterScopeMap, + layout, + }); - this.setState({ - activeFilterField: nextActiveFilterField, - filterScopeMap: { + setActiveFilterField(nextActiveFilterField); + setFilterScopeMap({ ...filterScopeMap, ...filterScopeTreeEntry, - } as FilterScopeMap, - }); - } - } + }); + } + }, + [ + activeFilterField, + allFilterFields, + checkedFilterFields, + filterScopeMap, + layout, + ], + ); - onSearchInputChange(e: ChangeEvent): void { - this.setState({ searchText: e.target.value }, this.filterTree); - } + const onSearchInputChange = useCallback( + (e: ChangeEvent): void => { + const newSearchText = e.target.value; + setSearchText(newSearchText); + filterTree(newSearchText); + }, + [filterTree], + ); - onClose(): void { - this.props.onCloseModal(); - } + const onClose = useCallback((): void => { + onCloseModal(); + }, [onCloseModal]); - onSave(): void { - const state = this.state as FilterScopeSelectorStateWithSelector; - const { filterScopeMap } = state; - - const allFilterFieldScopes = this.allfilterFields.reduce< + const onSave = useCallback((): void => { + const allFilterFieldScopes = allFilterFields.reduce< Record> >((map, filterKey) => { const { nodes } = filterScopeMap[filterKey]; @@ -669,124 +737,32 @@ export default class FilterScopeSelector extends PureComponent< }; }, {}); - this.props.updateDashboardFiltersScope(allFilterFieldScopes); - this.props.setUnsavedChanges(true); + updateDashboardFiltersScope(allFilterFieldScopes); + setUnsavedChanges(true); // click Save button will do save and close modal - this.props.onCloseModal(); - } + onCloseModal(); + }, [ + allFilterFields, + filterScopeMap, + onCloseModal, + setUnsavedChanges, + updateDashboardFiltersScope, + ]); - filterTree(): void { - const state = this.state as FilterScopeSelectorStateWithSelector; - // Reset nodes back to unfiltered state - if (!state.searchText) { - this.setState(prevState => { - const prev = prevState as FilterScopeSelectorStateWithSelector; - const { activeFilterField, checkedFilterFields, filterScopeMap } = prev; - const key = getKeyForFilterScopeTree({ - activeFilterField: activeFilterField ?? undefined, - checkedFilterFields, - }); - - const updatedEntry = { - ...filterScopeMap[key], - nodesFiltered: filterScopeMap[key].nodes, - }; - return { - filterScopeMap: { - ...filterScopeMap, - [key]: updatedEntry, - }, - } as Partial as FilterScopeSelectorState; - }); - } else { - const updater = ( - prevState: FilterScopeSelectorState, - ): FilterScopeSelectorState => { - const prev = prevState as FilterScopeSelectorStateWithSelector; - const { activeFilterField, checkedFilterFields, filterScopeMap } = prev; - const key = getKeyForFilterScopeTree({ - activeFilterField: activeFilterField ?? undefined, - checkedFilterFields, - }); - - const nodesFiltered = filterScopeMap[key].nodes.reduce< - FilterScopeTreeNode[] - >(this.filterNodes, []); - const expanded = getFilterScopeParentNodes([...nodesFiltered]); - const updatedEntry = { - ...filterScopeMap[key], - nodesFiltered, - expanded, - }; - - return { - filterScopeMap: { - ...filterScopeMap, - [key]: updatedEntry, - }, - } as Partial as FilterScopeSelectorState; - }; - - this.setState(updater); - } - } - - filterNodes( - filtered: FilterScopeTreeNode[] = [], - node: FilterScopeTreeNode = { value: '', label: '' }, - ): FilterScopeTreeNode[] { - const state = this.state as FilterScopeSelectorStateWithSelector; - const { searchText } = state; - const children = (node.children || []).reduce( - this.filterNodes, - [], - ); - - if ( - // Node's label matches the search string - node.label - .toLocaleLowerCase() - .indexOf((searchText ?? '').toLocaleLowerCase()) > -1 || - // Or a children has a matching node - children.length - ) { - filtered.push({ ...node, children }); - } - - return filtered; - } - - renderFilterFieldList(): ReactElement | null { - const state = this.state as FilterScopeSelectorStateWithSelector; - const { - activeFilterField, - filterFieldNodes, - checkedFilterFields, - expandedFilterIds, - } = state; - return ( - - ); - } - - renderFilterScopeTree(): ReactElement { - const state = this.state as FilterScopeSelectorStateWithSelector; - const { - filterScopeMap, - activeFilterField, - checkedFilterFields, - searchText, - } = state; + const renderFilterFieldList = (): ReactElement | null => ( + + ); + const renderFilterScopeTree = (): ReactElement => { const key = getKeyForFilterScopeTree({ activeFilterField: activeFilterField ?? undefined, checkedFilterFields, @@ -803,26 +779,23 @@ export default class FilterScopeSelector extends PureComponent< placeholder={t('Search...')} type="text" value={searchText} - onChange={this.onSearchInputChange} + onChange={onSearchInputChange} /> ); - } + }; - renderEditingFiltersName(): ReactElement { - const { dashboardFilters } = this.props; - const state = this.state as FilterScopeSelectorStateWithSelector; - const { activeFilterField, checkedFilterFields } = state; + const renderEditingFiltersName = (): ReactElement => { const currentFilterLabels = ([] as string[]) .concat(activeFilterField || checkedFilterFields) .filter(Boolean) @@ -842,50 +815,42 @@ export default class FilterScopeSelector extends PureComponent< ); - } + }; - render(): ReactElement { - const { showSelector } = this.state; + return ( + + +

{t('Configure filter scopes')}

+ {showSelector && renderEditingFiltersName()} +
- return ( - - -

{t('Configure filter scopes')}

- {showSelector && this.renderEditingFiltersName()} -
- - - {!showSelector ? ( -
- {t('There are no filters in this dashboard.')} + + {!showSelector ? ( +
+ {t('There are no filters in this dashboard.')} +
+ ) : ( + +
+ {renderFilterFieldList()}
- ) : ( - -
- {this.renderFilterFieldList()} -
-
- {this.renderFilterScopeTree()} -
-
- )} -
+
+ {renderFilterScopeTree()} +
+ + )} + - - + {showSelector && ( + - {showSelector && ( - - )} - - - ); - } + )} + + + ); } diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx b/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx index 07e234b5ff2..0f810ac36e3 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx @@ -763,11 +763,11 @@ const Chart = (props: ChartProps) => { }, slice.viz_type, )} - queriesResponse={chart.queriesResponse ?? undefined} + queriesResponse={chart.queriesResponse ?? null} timeout={timeout} triggerQuery={chart.triggerQuery} vizType={slice.viz_type} - setControlValue={props.setControlValue} + setControlValue={props.setControlValue ?? (() => {})} datasetsStatus={ datasetsStatus as 'loading' | 'error' | 'complete' | undefined } @@ -775,7 +775,6 @@ const Chart = (props: ChartProps) => { emitCrossFilters={emitCrossFilters} onChartStateChange={handleChartStateChange} suppressLoadingSpinner={suppressLoadingSpinner} - filterState={dataMask[props.id]?.filterState} /> diff --git a/superset-frontend/src/dashboard/components/gridComponents/Divider/Divider.tsx b/superset-frontend/src/dashboard/components/gridComponents/Divider/Divider.tsx index f6b26b48928..f597feabca0 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Divider/Divider.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Divider/Divider.tsx @@ -17,8 +17,8 @@ * under the License. */ -import { PureComponent } from 'react'; -import { css, styled } from '@apache-superset/core/theme'; +import { useCallback, memo } from 'react'; +import { css, styled } from '@apache-superset/core/ui'; import { Draggable } from '../../dnd/DragDroppable'; import HoverMenu from '../../menu/HoverMenu'; @@ -63,50 +63,43 @@ const DividerLine = styled.div` `} `; -class Divider extends PureComponent { - constructor(props: DividerProps) { - super(props); - this.handleDeleteComponent = this.handleDeleteComponent.bind(this); - } - - handleDeleteComponent() { - const { deleteComponent, id, parentId } = this.props; +function Divider({ + id, + parentId, + component, + depth, + parentComponent, + index, + editMode, + handleComponentDrop, + deleteComponent, +}: DividerProps) { + const handleDeleteComponent = useCallback(() => { deleteComponent(id, parentId); - } + }, [deleteComponent, id, parentId]); - render() { - const { - component, - depth, - parentComponent, - index, - handleComponentDrop, - editMode, - } = this.props; - - return ( - - {({ dragSourceRef }: { dragSourceRef: ConnectDragSource }) => ( -
- {editMode && ( - - - - )} - -
- )} -
- ); - } + return ( + + {({ dragSourceRef }: { dragSourceRef: ConnectDragSource }) => ( +
+ {editMode && ( + + + + )} + +
+ )} +
+ ); } -export default Divider; +export default memo(Divider); diff --git a/superset-frontend/src/dashboard/components/gridComponents/Header/Header.tsx b/superset-frontend/src/dashboard/components/gridComponents/Header/Header.tsx index dd9b2a6a6d6..76750c8fc5c 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Header/Header.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Header/Header.tsx @@ -16,9 +16,9 @@ * specific language governing permissions and limitations * under the License. */ -import { PureComponent } from 'react'; +import { useState, useCallback, memo } from 'react'; import cx from 'classnames'; -import { css, styled } from '@apache-superset/core/theme'; +import { css, styled } from '@apache-superset/core/ui'; import PopoverDropdown from '@superset-ui/core/components/PopoverDropdown'; import { EditableTitle } from '@superset-ui/core/components'; @@ -85,10 +85,6 @@ interface HeaderProps { updateComponents: (changes: Record) => void; } -interface HeaderState { - isFocused: boolean; -} - const HeaderStyles = styled.div` ${({ theme }) => css` font-weight: ${theme.fontWeightStrong}; @@ -159,149 +155,141 @@ const HeaderStyles = styled.div` `} `; -class Header extends PureComponent { - handleChangeSize: (nextValue: string) => void; - handleChangeBackground: (nextValue: string) => void; - handleChangeText: (nextValue: string) => void; +function Header({ + id, + dashboardId, + parentId, + component, + depth, + parentComponent, + index, + editMode, + embeddedMode, + handleComponentDrop, + deleteComponent, + updateComponents, +}: HeaderProps) { + const [isFocused, setIsFocused] = useState(false); - constructor(props: HeaderProps) { - super(props); - this.state = { - isFocused: false, - }; - this.handleDeleteComponent = this.handleDeleteComponent.bind(this); - this.handleChangeFocus = this.handleChangeFocus.bind(this); - this.handleUpdateMeta = this.handleUpdateMeta.bind(this); + const handleChangeFocus = useCallback((nextFocus: boolean): void => { + setIsFocused(nextFocus); + }, []); - this.handleChangeSize = (nextValue: string) => - this.handleUpdateMeta('headerSize', nextValue); - this.handleChangeBackground = (nextValue: string) => - this.handleUpdateMeta('background', nextValue); - this.handleChangeText = (nextValue: string) => - this.handleUpdateMeta('text', nextValue); - } - - handleChangeFocus(nextFocus: boolean): void { - this.setState(() => ({ isFocused: nextFocus })); - } - - handleUpdateMeta(metaKey: keyof ComponentMeta, nextValue: string): void { - const { updateComponents, component } = this.props; - if (nextValue && component.meta[metaKey] !== nextValue) { - updateComponents({ - [component.id]: { - ...component, - meta: { - ...component.meta, - [metaKey]: nextValue, + const handleUpdateMeta = useCallback( + (metaKey: keyof ComponentMeta, nextValue: string): void => { + if (nextValue && component.meta[metaKey] !== nextValue) { + updateComponents({ + [component.id]: { + ...component, + meta: { + ...component.meta, + [metaKey]: nextValue, + }, }, - }, - } as Record); - } - } + } as Record); + } + }, + [component, updateComponents], + ); - handleDeleteComponent(): void { - const { deleteComponent, id, parentId } = this.props; + const handleChangeSize = useCallback( + (nextValue: string) => handleUpdateMeta('headerSize', nextValue), + [handleUpdateMeta], + ); + + const handleChangeBackground = useCallback( + (nextValue: string) => handleUpdateMeta('background', nextValue), + [handleUpdateMeta], + ); + + const handleChangeText = useCallback( + (nextValue: string) => handleUpdateMeta('text', nextValue), + [handleUpdateMeta], + ); + + const handleDeleteComponent = useCallback((): void => { deleteComponent(id, parentId); - } + }, [deleteComponent, id, parentId]); - render() { - const { isFocused } = this.state; + const headerStyle = headerStyleOptions.find( + opt => opt.value === (component.meta.headerSize || SMALL_HEADER), + ); - const { - dashboardId, - component, - depth, - parentComponent, - index, - handleComponentDrop, - editMode, - embeddedMode, - } = this.props; + const rowStyle = backgroundStyleOptions.find( + opt => opt.value === (component.meta.background || BACKGROUND_TRANSPARENT), + ); - const headerStyle = headerStyleOptions.find( - opt => opt.value === (component.meta.headerSize || SMALL_HEADER), - ); - - const rowStyle = backgroundStyleOptions.find( - opt => - opt.value === (component.meta.background || BACKGROUND_TRANSPARENT), - ); - - return ( - - {({ - dragSourceRef, - }: { - dragSourceRef: React.Ref | undefined; - }) => ( -
- {editMode && - depth <= 2 && ( // drag handle looks bad when nested - - + return ( + + {({ + dragSourceRef, + }: { + dragSourceRef: React.Ref | undefined; + }) => ( +
+ {editMode && + depth <= 2 && ( // drag handle looks bad when nested + + + + )} + , + , + ]} + editMode={editMode} + > + + {editMode && ( + + )} - , - , - ]} - editMode={editMode} - > - - {editMode && ( - - - - )} - + {!editMode && !embeddedMode && ( + - {!editMode && !embeddedMode && ( - - )} - - -
- )} -
- ); - } + )} + + +
+ )} +
+ ); } -export default Header; +export default memo(Header); diff --git a/superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.tsx b/superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.tsx index 55f7426348d..31fd5936271 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.tsx @@ -16,14 +16,14 @@ * specific language governing permissions and limitations * under the License. */ -import { PureComponent } from 'react'; +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { connect } from 'react-redux'; import cx from 'classnames'; import type { JsonObject } from '@superset-ui/core'; import type { ResizeStartCallback, ResizeCallback } from 're-resizable'; +import { ErrorBoundary } from 'src/components'; -import { css, styled } from '@apache-superset/core/theme'; -import { t } from '@apache-superset/core/translation'; +import { t, css, styled } from '@apache-superset/core/ui'; import { SafeMarkdown } from '@superset-ui/core/components'; import { EditorHost } from 'src/core/editors'; import { Logger, LOG_ACTIONS_RENDER_CHART } from 'src/logger/LogUtils'; @@ -82,16 +82,6 @@ export interface MarkdownStateProps { export type MarkdownProps = MarkdownOwnProps & MarkdownStateProps; -export interface MarkdownState { - isFocused: boolean; - markdownSource: string; - editor: EditorInstance | null; - editorMode: 'preview' | 'edit'; - undoLength: number; - redoLength: number; - hasError?: boolean; -} - // TODO: localize const MARKDOWN_PLACE_HOLDER = `# ✨Header 1 ## ✨Header 2 @@ -140,193 +130,200 @@ interface DragChildProps { dragSourceRef: React.RefCallback; } -class Markdown extends PureComponent { - renderStartTime: number; +function Markdown({ + id, + parentId, + component, + parentComponent, + index, + depth, + editMode, + availableColumnCount, + columnWidth, + onResizeStart, + onResize, + onResizeStop, + deleteComponent, + handleComponentDrop, + updateComponents, + logEvent, + addDangerToast, + undoLength, + redoLength, + htmlSanitization, + htmlSchemaOverrides, +}: MarkdownProps) { + const [isFocused, setIsFocused] = useState(false); + const [markdownSource, setMarkdownSource] = useState( + component.meta.code as string, + ); + const [editor, setEditorState] = useState(null); + const [editorMode, setEditorMode] = useState<'preview' | 'edit'>('preview'); + const [hasError, setHasError] = useState(false); - constructor(props: MarkdownProps) { - super(props); - this.state = { - isFocused: false, - markdownSource: props.component.meta.code as string, - editor: null, - editorMode: 'preview', - undoLength: props.undoLength, - redoLength: props.redoLength, - }; - this.renderStartTime = Logger.getTimestamp(); + const renderStartTimeRef = useRef(Logger.getTimestamp()); + const prevUndoLengthRef = useRef(undoLength); + const prevRedoLengthRef = useRef(redoLength); + const prevComponentWidthRef = useRef(component.meta.width); + const prevColumnWidthRef = useRef(columnWidth); - this.handleChangeFocus = this.handleChangeFocus.bind(this); - this.handleChangeEditorMode = this.handleChangeEditorMode.bind(this); - this.handleMarkdownChange = this.handleMarkdownChange.bind(this); - this.handleDeleteComponent = this.handleDeleteComponent.bind(this); - this.handleResizeStart = this.handleResizeStart.bind(this); - this.setEditor = this.setEditor.bind(this); - this.shouldFocusMarkdown = this.shouldFocusMarkdown.bind(this); - } - - componentDidMount(): void { - this.props.logEvent(LOG_ACTIONS_RENDER_CHART, { - viz_type: 'markdown', - start_offset: this.renderStartTime, - ts: new Date().getTime(), - duration: Logger.getTimestamp() - this.renderStartTime, - }); - } - - static getDerivedStateFromProps( - nextProps: MarkdownProps, - state: MarkdownState, - ): MarkdownState | null { - const { hasError, editorMode, markdownSource, undoLength, redoLength } = - state; - const { - component: nextComponent, - undoLength: nextUndoLength, - redoLength: nextRedoLength, - } = nextProps; - // user click undo or redo ? - if (nextUndoLength !== undoLength || nextRedoLength !== redoLength) { - return { - ...state, - undoLength: nextUndoLength, - redoLength: nextRedoLength, - markdownSource: nextComponent.meta.code as string, - hasError: false, - }; - } + // getDerivedStateFromProps equivalent: handle undo/redo and external code changes + useEffect(() => { + // user click undo or redo? if ( + undoLength !== prevUndoLengthRef.current || + redoLength !== prevRedoLengthRef.current + ) { + setMarkdownSource(component.meta.code as string); + setHasError(false); + prevUndoLengthRef.current = undoLength; + prevRedoLengthRef.current = redoLength; + } else if ( !hasError && editorMode === 'preview' && - nextComponent.meta.code !== markdownSource + component.meta.code !== markdownSource ) { - return { - ...state, - markdownSource: nextComponent.meta.code as string, - }; + setMarkdownSource(component.meta.code as string); } + }, [ + undoLength, + redoLength, + component.meta.code, + hasError, + editorMode, + markdownSource, + ]); - return state; - } + // componentDidMount equivalent: log render event + useEffect(() => { + logEvent(LOG_ACTIONS_RENDER_CHART, { + viz_type: 'markdown', + start_offset: renderStartTimeRef.current, + ts: new Date().getTime(), + duration: Logger.getTimestamp() - renderStartTimeRef.current, + }); + // Only run on mount + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); - static getDerivedStateFromError(): { hasError: boolean } { - return { - hasError: true, - }; - } - - componentDidUpdate(prevProps: MarkdownProps): void { + // componentDidUpdate equivalent: resize editor when width changes + useEffect(() => { if ( - this.state.editor && - (prevProps.component.meta.width !== this.props.component.meta.width || - prevProps.columnWidth !== this.props.columnWidth) + editor && + (prevComponentWidthRef.current !== component.meta.width || + prevColumnWidthRef.current !== columnWidth) ) { // Handle both Ace editor (resize method) and EditorHandle (no resize needed) - if (typeof this.state.editor.resize === 'function') { - this.state.editor.resize(true); + if (typeof editor.resize === 'function') { + editor.resize(true); } } - } + prevComponentWidthRef.current = component.meta.width; + prevColumnWidthRef.current = columnWidth; + }, [editor, component.meta.width, columnWidth]); - componentDidCatch(): void { - if (this.state.editor && this.state.editorMode === 'preview') { - this.props.addDangerToast( - t( - 'This markdown component has an error. Please revert your recent changes.', - ), - ); - } - } - - setEditor(editor: EditorInstance): void { - // EditorHandle or Ace editor instance - // For Ace: editor.getSession().setUseWrapMode(true) - // For EditorHandle: wrapEnabled is handled via options - if (editor?.getSession) { - editor.getSession!().setUseWrapMode(true); - } - this.setState({ - editor, - }); - } - - handleChangeFocus(nextFocus: boolean | number): void { - const nextFocused = !!nextFocus; - const nextEditMode: 'edit' | 'preview' = nextFocused ? 'edit' : 'preview'; - this.setState(() => ({ isFocused: nextFocused })); - this.handleChangeEditorMode(nextEditMode); - } - - handleChangeEditorMode(mode: 'edit' | 'preview'): void { - const nextState: MarkdownState = { - ...this.state, - editorMode: mode, - }; - if (mode === 'preview') { - this.updateMarkdownContent(); - nextState.hasError = false; - } - - this.setState(nextState); - } - - updateMarkdownContent(): void { - const { updateComponents, component } = this.props; - if (component.meta.code !== this.state.markdownSource) { + const updateMarkdownContent = useCallback((): void => { + if (component.meta.code !== markdownSource) { updateComponents({ [component.id]: { ...component, meta: { ...component.meta, - code: this.state.markdownSource, + code: markdownSource, }, }, }); } - } + }, [component, markdownSource, updateComponents]); - handleMarkdownChange(nextValue: string): void { - this.setState({ - markdownSource: nextValue, - }); - } - - handleDeleteComponent(): void { - const { deleteComponent, id, parentId } = this.props; - deleteComponent(id, parentId); - } - - handleResizeStart(...args: Parameters): void { - const { editorMode } = this.state; - const { editMode, onResizeStart } = this.props; - const isEditing = editorMode === 'edit'; - onResizeStart(...args); - if (editMode && isEditing) { - this.updateMarkdownContent(); + const setEditor = useCallback((editorInstance: EditorInstance): void => { + // EditorHandle or Ace editor instance + // For Ace: editor.getSession().setUseWrapMode(true) + // For EditorHandle: wrapEnabled is handled via options + if (editorInstance?.getSession) { + editorInstance.getSession!().setUseWrapMode(true); } - } + setEditorState(editorInstance); + }, []); - shouldFocusMarkdown( - event: MouseEvent, - container: HTMLElement | null, - menuRef: HTMLElement | null, - ): boolean { - if (container?.contains(event.target as Node)) return true; - if (menuRef?.contains(event.target as Node)) return true; + const handleChangeEditorMode = useCallback( + (mode: 'edit' | 'preview'): void => { + if (mode === 'preview') { + updateMarkdownContent(); + setHasError(false); + } + setEditorMode(mode); + }, + [updateMarkdownContent], + ); - return false; - } + const handleChangeFocus = useCallback( + (nextFocus: boolean | number): void => { + const nextFocused = !!nextFocus; + const nextEditMode: 'edit' | 'preview' = nextFocused ? 'edit' : 'preview'; + setIsFocused(nextFocused); + handleChangeEditorMode(nextEditMode); + }, + [handleChangeEditorMode], + ); - renderEditMode(): JSX.Element { - return ( + const handleMarkdownChange = useCallback((nextValue: string): void => { + setMarkdownSource(nextValue); + }, []); + + const handleDeleteComponent = useCallback((): void => { + deleteComponent(id, parentId); + }, [deleteComponent, id, parentId]); + + const handleResizeStart = useCallback( + (...args: Parameters): void => { + const isEditing = editorMode === 'edit'; + onResizeStart(...args); + if (editMode && isEditing) { + updateMarkdownContent(); + } + }, + [editorMode, editMode, onResizeStart, updateMarkdownContent], + ); + + const shouldFocusMarkdown = useCallback( + ( + event: MouseEvent, + container: HTMLElement | null, + menuRef: HTMLElement | null, + ): boolean => { + if (container?.contains(event.target as Node)) return true; + if (menuRef?.contains(event.target as Node)) return true; + return false; + }, + [], + ); + + const handleRenderError = useCallback( + (error: Error, info: { componentStack: string } | null): void => { + setHasError(true); + if (editorMode === 'preview') { + addDangerToast( + t( + 'This markdown component has an error. Please revert your recent changes.', + ), + ); + } + }, + [addDangerToast, editorMode], + ); + + const renderEditMode = useMemo( + () => ( delete" to give an empty editor - typeof this.state.markdownSource === 'string' - ? this.state.markdownSource + typeof markdownSource === 'string' + ? markdownSource : MARKDOWN_PLACE_HOLDER } language="markdown" @@ -336,126 +333,122 @@ class Markdown extends PureComponent { onReady={(handle: EditorInstance) => { // The handle provides access to the underlying editor for resize if (handle && typeof handle.focus === 'function') { - this.setEditor(handle); + setEditor(handle); } }} data-test="editor" /> - ); - } + ), + [id, markdownSource, handleMarkdownChange, setEditor], + ); - renderPreviewMode(): JSX.Element { - const { hasError } = this.state; - - return ( - - ); - } - - render() { - const { isFocused, editorMode } = this.state; - - const { - component, - parentComponent, - index, - depth, - availableColumnCount, - columnWidth, - onResize, - onResizeStop, - handleComponentDrop, - editMode, - } = this.props; - - // inherit the size of parent columns - const widthMultiple = - parentComponent.type === COLUMN_TYPE - ? parentComponent.meta.width || GRID_MIN_COLUMN_COUNT - : component.meta.width || GRID_MIN_COLUMN_COUNT; - - const isEditing = editorMode === 'edit'; - - return ( - ( + - {({ dragSourceRef }: DragChildProps) => ( - , - ]} - editMode={editMode} + + + ), + [ + hasError, + markdownSource, + htmlSanitization, + htmlSchemaOverrides, + handleRenderError, + ], + ); + + // inherit the size of parent columns + const widthMultiple = + parentComponent.type === COLUMN_TYPE + ? parentComponent.meta.width || GRID_MIN_COLUMN_COUNT + : component.meta.width || GRID_MIN_COLUMN_COUNT; + + const isEditing = editorMode === 'edit'; + + const menuItems = useMemo( + () => [ + , + ], + [component.id, editorMode, handleChangeEditorMode], + ); + + return ( + + {({ dragSourceRef }: DragChildProps) => ( + + - - -
- {editMode && ( - - - - )} - {editMode && isEditing - ? this.renderEditMode() - : this.renderPreviewMode()} -
-
-
-
- )} -
- ); - } + {editMode && ( + + + + )} + {editMode && isEditing ? renderEditMode : renderPreviewMode} +
+ + + + )} + + ); } interface ReduxState { diff --git a/superset-frontend/src/dashboard/components/gridComponents/new/DraggableNewComponent.tsx b/superset-frontend/src/dashboard/components/gridComponents/new/DraggableNewComponent.tsx index 8426521d4e0..0aa019b7d4d 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/new/DraggableNewComponent.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/new/DraggableNewComponent.tsx @@ -16,9 +16,9 @@ * specific language governing permissions and limitations * under the License. */ -import { PureComponent } from 'react'; +import { memo } from 'react'; import cx from 'classnames'; -import { css, styled } from '@apache-superset/core/theme'; +import { css, styled } from '@apache-superset/core/ui'; import { DragDroppable } from 'src/dashboard/components/dnd/DragDroppable'; import type { ConnectDragSource } from 'react-dnd'; import { NEW_COMPONENTS_SOURCE_ID } from 'src/dashboard/util/constants'; @@ -62,37 +62,37 @@ const NewComponentPlaceholder = styled.div` `} `; -export default class DraggableNewComponent extends PureComponent { - static defaultProps = { - className: null, - IconComponent: undefined, - }; - - render() { - const { label, id, type, className, meta, IconComponent } = this.props; - - return ( - - {({ dragSourceRef }: { dragSourceRef: ConnectDragSource }) => ( - - - {IconComponent && } - - {label} - - )} - - ); - } +function DraggableNewComponent({ + label, + id, + type, + className, + meta, + IconComponent, +}: DraggableNewComponentProps) { + return ( + + {({ dragSourceRef }: { dragSourceRef: ConnectDragSource }) => ( + + + {IconComponent && } + + {label} + + )} + + ); } + +export default memo(DraggableNewComponent); diff --git a/superset-frontend/src/dashboard/components/menu/BackgroundStyleDropdown.tsx b/superset-frontend/src/dashboard/components/menu/BackgroundStyleDropdown.tsx index 46d8eb2f5f3..94188095bb0 100644 --- a/superset-frontend/src/dashboard/components/menu/BackgroundStyleDropdown.tsx +++ b/superset-frontend/src/dashboard/components/menu/BackgroundStyleDropdown.tsx @@ -16,10 +16,9 @@ * specific language governing permissions and limitations * under the License. */ -import { PureComponent } from 'react'; import cx from 'classnames'; -import { t } from '@apache-superset/core/translation'; -import { css, styled } from '@apache-superset/core/theme'; +import { t } from '@apache-superset/core'; +import { css, styled } from '@apache-superset/core/ui'; import backgroundStyleOptions from 'src/dashboard/util/backgroundStyleOptions'; import PopoverDropdown, { @@ -90,18 +89,19 @@ function renderOption(option: OptionProps) { ); } -export default class BackgroundStyleDropdown extends PureComponent { - render() { - const { id, value, onChange } = this.props; - return ( - - ); - } +export default function BackgroundStyleDropdown({ + id, + value, + onChange, +}: BackgroundStyleDropdownProps) { + return ( + + ); } diff --git a/superset-frontend/src/dashboard/components/menu/HoverMenu.tsx b/superset-frontend/src/dashboard/components/menu/HoverMenu.tsx index 9e45c96b986..0a161de68b5 100644 --- a/superset-frontend/src/dashboard/components/menu/HoverMenu.tsx +++ b/superset-frontend/src/dashboard/components/menu/HoverMenu.tsx @@ -1,4 +1,3 @@ -/* eslint-disable react/no-unused-state */ /** * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file @@ -17,15 +16,15 @@ * specific language governing permissions and limitations * under the License. */ -import { RefObject, ReactNode, PureComponent } from 'react'; +import { RefObject, ReactNode, useCallback, memo } from 'react'; -import { styled } from '@apache-superset/core/theme'; +import { styled } from '@apache-superset/core/ui'; import cx from 'classnames'; interface HoverMenuProps { - position: 'left' | 'top'; - innerRef: RefObject; - children: ReactNode; + position?: 'left' | 'top'; + innerRef?: RefObject | null; + children?: ReactNode; onHover?: (data: { isHovered: boolean }) => void; } @@ -66,45 +65,41 @@ const HoverStyleOverrides = styled.div` } `; -export default class HoverMenu extends PureComponent { - static defaultProps = { - position: 'left', - innerRef: null, - children: null, - }; - - handleMouseEnter = () => { - const { onHover } = this.props; +function HoverMenu({ + position = 'left', + innerRef = null, + children = null, + onHover, +}: HoverMenuProps) { + const handleMouseEnter = useCallback(() => { if (onHover) { onHover({ isHovered: true }); } - }; + }, [onHover]); - handleMouseLeave = () => { - const { onHover } = this.props; + const handleMouseLeave = useCallback(() => { if (onHover) { onHover({ isHovered: false }); } - }; + }, [onHover]); - render() { - const { innerRef, position, children } = this.props; - return ( - -
- {children} -
-
- ); - } + return ( + +
+ {children} +
+
+ ); } + +export default memo(HoverMenu); diff --git a/superset-frontend/src/dashboard/components/menu/MarkdownModeDropdown.tsx b/superset-frontend/src/dashboard/components/menu/MarkdownModeDropdown.tsx index 8dccc4921df..c9800b565db 100644 --- a/superset-frontend/src/dashboard/components/menu/MarkdownModeDropdown.tsx +++ b/superset-frontend/src/dashboard/components/menu/MarkdownModeDropdown.tsx @@ -16,8 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { PureComponent } from 'react'; -import { t } from '@apache-superset/core/translation'; +import { t } from '@apache-superset/core'; import PopoverDropdown, { OnChangeHandler, @@ -40,18 +39,18 @@ const dropdownOptions = [ }, ]; -export default class MarkdownModeDropdown extends PureComponent { - render() { - const { id, value, onChange } = this.props; - - return ( - - ); - } +export default function MarkdownModeDropdown({ + id, + value, + onChange, +}: MarkdownModeDropdownProps) { + return ( + + ); } diff --git a/superset-frontend/src/dashboard/components/menu/WithPopoverMenu.test.tsx b/superset-frontend/src/dashboard/components/menu/WithPopoverMenu.test.tsx index 97c0f0bf2fa..1e391b720ff 100644 --- a/superset-frontend/src/dashboard/components/menu/WithPopoverMenu.test.tsx +++ b/superset-frontend/src/dashboard/components/menu/WithPopoverMenu.test.tsx @@ -106,7 +106,9 @@ test('should unfocus when another component is clicked', async () => { container?.contains(event.target)} + shouldFocus={(event, container, _menuRef) => + container?.contains(event.target) ?? false + } onChangeFocus={onChangeFocusA} >
@@ -117,7 +119,9 @@ test('should unfocus when another component is clicked', async () => { container?.contains(event.target)} + shouldFocus={(event, container, _menuRef) => + container?.contains(event.target) ?? false + } onChangeFocus={onChangeFocusB} >
diff --git a/superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx b/superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx index 3fb061ed15b..a71ef1667c0 100644 --- a/superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx +++ b/superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx @@ -16,36 +16,50 @@ * specific language governing permissions and limitations * under the License. */ -import { ReactNode, CSSProperties, PureComponent } from 'react'; +import { + ReactNode, + CSSProperties, + useCallback, + useEffect, + useRef, + useState, + memo, +} from 'react'; import cx from 'classnames'; import { addAlpha } from '@superset-ui/core'; -import { css, styled } from '@apache-superset/core/theme'; +import { css, styled } from '@apache-superset/core/ui'; type ShouldFocusContainer = HTMLDivElement & { contains: (event_target: EventTarget & HTMLElement) => boolean; }; interface WithPopoverMenuProps { - children: ReactNode; - disableClick: boolean; - menuItems: ReactNode[]; - onChangeFocus: (focus: boolean) => void; - isFocused: boolean; - // Event argument is left as "any" because of the clash. In defaultProps it seems + children?: ReactNode; + disableClick?: boolean; + menuItems?: ReactNode[]; + onChangeFocus?: ((focus: boolean) => void) | null; + isFocused?: boolean; + // Event argument is left as "any" because of the clash. In props it seems // like it should be React.FocusEvent<>, however from handleClick() we can also // derive that type is EventListenerOrEventListenerObject. - shouldFocus: ( + shouldFocus?: ( event: any, - container: ShouldFocusContainer, + container: ShouldFocusContainer | null, menuRef: HTMLDivElement | null, ) => boolean; - editMode: boolean; - style: CSSProperties; + editMode?: boolean; + style?: CSSProperties | null; } -interface WithPopoverMenuState { - isFocused: boolean; -} +const defaultShouldFocus = ( + event: any, + container: ShouldFocusContainer | null, + menuRef: HTMLDivElement | null, +): boolean => { + if (container?.contains(event.target)) return true; + if (menuRef?.contains(event.target)) return true; + return false; +}; const WithPopoverMenuStyles = styled.div` ${({ theme }) => css` @@ -104,151 +118,90 @@ const PopoverMenuStyles = styled.div` `} `; -export default class WithPopoverMenu extends PureComponent< - WithPopoverMenuProps, - WithPopoverMenuState -> { - container: ShouldFocusContainer; +function WithPopoverMenu({ + children = null, + disableClick = false, + menuItems = [], + onChangeFocus = null, + isFocused: isFocusedProp = false, + shouldFocus: shouldFocusFunc = defaultShouldFocus, + editMode = false, + style = null, +}: WithPopoverMenuProps) { + const [isFocused, setIsFocused] = useState(isFocusedProp); + const containerRef = useRef(null); + const menuRef = useRef(null); - menuRef: HTMLDivElement | null; + const handleClick = useCallback( + (event: any) => { + if (!editMode) { + return; + } - focusEvent: Event | null; + const shouldFocusResult = shouldFocusFunc( + event, + containerRef.current, + menuRef.current, + ); - static defaultProps = { - children: null, - disableClick: false, - onChangeFocus: null, - menuItems: [], - isFocused: false, - shouldFocus: ( - event: any, - container: ShouldFocusContainer, - menuRef: HTMLDivElement | null, - ) => { - if (container?.contains(event.target)) return true; - if (menuRef?.contains(event.target)) return true; - return false; + if (shouldFocusResult === isFocused) return; + + if (!disableClick && shouldFocusResult && !isFocused) { + setIsFocused(true); + if (onChangeFocus) onChangeFocus(true); + } else if (!shouldFocusResult && isFocused) { + setIsFocused(false); + if (onChangeFocus) onChangeFocus(false); + } }, - style: null, - }; + [editMode, shouldFocusFunc, isFocused, disableClick, onChangeFocus], + ); - constructor(props: WithPopoverMenuProps) { - super(props); - this.state = { - isFocused: props.isFocused!, + // Handle prop-driven focus changes and add/remove document listeners + useEffect(() => { + if (editMode && isFocusedProp && !isFocused) { + setIsFocused(true); + } else if (isFocused && !editMode) { + setIsFocused(false); + } + }, [editMode, isFocusedProp, isFocused]); + + // Add/remove document event listeners based on focus state + useEffect(() => { + if (isFocused && editMode) { + document.addEventListener('click', handleClick); + document.addEventListener('drag', handleClick); + } + + return () => { + document.removeEventListener('click', handleClick); + document.removeEventListener('drag', handleClick); }; - this.menuRef = null; - this.focusEvent = null; - this.setRef = this.setRef.bind(this); - this.setMenuRef = this.setMenuRef.bind(this); - this.handleClick = this.handleClick.bind(this); - } + }, [isFocused, editMode, handleClick]); - componentDidUpdate(prevProps: WithPopoverMenuProps) { - if (this.props.editMode && this.props.isFocused && !this.state.isFocused) { - document.addEventListener('click', this.handleClick); - document.addEventListener('drag', this.handleClick); - this.setState({ isFocused: true }); - } else if (this.state.isFocused && !this.props.editMode) { - document.removeEventListener('click', this.handleClick); - document.removeEventListener('drag', this.handleClick); - this.setState({ isFocused: false }); - } - } - - componentWillUnmount() { - document.removeEventListener('click', this.handleClick); - document.removeEventListener('drag', this.handleClick); - } - - setRef(ref: ShouldFocusContainer) { - this.container = ref; - } - - setMenuRef(ref: HTMLDivElement | null) { - this.menuRef = ref; - } - - shouldHandleFocusChange(shouldFocus: boolean): boolean { - const { disableClick } = this.props; - const { isFocused } = this.state; - - return ( - (!disableClick && shouldFocus && !isFocused) || - (!shouldFocus && isFocused) - ); - } - - handleClick(event: any) { - if (!this.props.editMode) { - return; - } - - // Skip if this is the same event that just triggered focus via onClick. - // The document-level listener registered during focus will see the same - // event bubble up; by that time a re-render may have detached the - // original event.target, causing shouldFocus to return false and - // immediately undoing the focus. - const nativeEvent = event.nativeEvent || event; - if (this.focusEvent === nativeEvent) { - this.focusEvent = null; - return; - } - - const { - onChangeFocus, - shouldFocus: shouldFocusFunc, - disableClick, - } = this.props; - - const shouldFocus = shouldFocusFunc(event, this.container, this.menuRef); - - if (shouldFocus === this.state.isFocused) return; - - if (!disableClick && shouldFocus && !this.state.isFocused) { - document.addEventListener('click', this.handleClick); - document.addEventListener('drag', this.handleClick); - this.focusEvent = event.nativeEvent || event; - - this.setState(() => ({ isFocused: true })); - - if (onChangeFocus) onChangeFocus(true); - } else if (!shouldFocus && this.state.isFocused) { - document.removeEventListener('click', this.handleClick); - document.removeEventListener('drag', this.handleClick); - - this.setState(() => ({ isFocused: false })); - - if (onChangeFocus) onChangeFocus(false); - } - } - - render() { - const { children, menuItems, editMode, style } = this.props; - const { isFocused } = this.state; - - return ( - - {children} - {editMode && isFocused && (menuItems?.length ?? 0) > 0 && ( - - {menuItems.map((node: ReactNode, i: number) => ( -
- {node} -
- ))} -
- )} -
- ); - } + return ( + + {children} + {editMode && isFocused && (menuItems?.length ?? 0) > 0 && ( + + {menuItems.map((node: ReactNode, i: number) => ( +
+ {node} +
+ ))} +
+ )} +
+ ); } + +export default memo(WithPopoverMenu); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.test.tsx new file mode 100644 index 00000000000..8bc8239f7e9 --- /dev/null +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.test.tsx @@ -0,0 +1,220 @@ +/** + * 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, waitFor } from 'spec/helpers/testing-library'; +import { Provider } from 'react-redux'; +import configureStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import { NativeFilterType } from '@superset-ui/core'; +import type { Filter } from '@superset-ui/core'; +import FilterValue from './FilterValue'; + +const mockGetChartDataRequest = jest.fn(); +jest.mock('src/components/Chart/chartAction', () => ({ + getChartDataRequest: (...args: unknown[]) => mockGetChartDataRequest(...args), +})); + +jest.mock('src/middleware/asyncEvent', () => ({ + waitForAsyncData: jest.fn(), +})); + +jest.mock('@superset-ui/core', () => { + const original = jest.requireActual('@superset-ui/core'); + return { + ...original, + getChartMetadataRegistry: () => ({ + get: () => ({ enableNoResults: false }), + }), + SuperChart: (props: Record) => ( +
+ SuperChart +
+ ), + isFeatureEnabled: () => false, + getClientErrorObject: (err: unknown) => + Promise.resolve({ + message: 'Something went wrong', + errors: [ + { message: 'Test error', error_type: 'GENERIC_BACKEND_ERROR' }, + ], + }), + }; +}); + +jest.mock('../useFilterOutlined', () => ({ + useFilterOutlined: () => ({ + outlinedFilterId: undefined, + lastUpdated: 0, + }), +})); + +const mockUseFilterDependencies = jest.fn().mockReturnValue({}); +jest.mock('./state', () => ({ + useFilterDependencies: (...args: unknown[]) => + mockUseFilterDependencies(...args), +})); + +const mockStore = configureStore([thunk]); + +const createMockFilter = (overrides: Partial = {}): Filter => ({ + id: 'NATIVE_FILTER-1', + name: 'Test Filter', + filterType: 'filter_select', + targets: [{ datasetId: 1, column: { name: 'country' } }], + defaultDataMask: {}, + controlValues: {}, + cascadeParentIds: [], + scope: { rootPath: ['ROOT_ID'], excluded: [] }, + type: NativeFilterType.NativeFilter, + description: 'Test filter description', + ...overrides, +}); + +const getDefaultStoreState = () => ({ + dashboardInfo: { id: 1 }, + dashboardState: { + isRefreshing: false, + isFiltersRefreshing: false, + directPathToChild: [], + directPathLastUpdated: 0, + }, + nativeFilters: { + filters: { + 'NATIVE_FILTER-1': createMockFilter(), + }, + filterSets: {}, + }, + dataMask: {}, + charts: {}, + dashboardLayout: { present: {} }, +}); + +const defaultProps = { + filter: createMockFilter(), + dataMaskSelected: {}, + onFilterSelectionChange: jest.fn(), + inView: true, +}; + +function renderFilterValue( + propOverrides: Record = {}, + stateOverrides: Record = {}, +) { + const state = { ...getDefaultStoreState(), ...stateOverrides }; + const store = mockStore(state); + const mergedProps = { ...defaultProps, ...propOverrides }; + return render( + + + , + ); +} + +beforeEach(() => { + jest.clearAllMocks(); +}); + +test('renders loading spinner when filter has a data source', () => { + mockGetChartDataRequest.mockReturnValue(new Promise(() => {})); + + renderFilterValue(); + + expect(screen.getByRole('status')).toBeInTheDocument(); + expect(screen.queryByTestId('mock-super-chart')).not.toBeInTheDocument(); +}); + +test('renders SuperChart after data loads successfully', async () => { + mockGetChartDataRequest.mockResolvedValue({ + response: { status: 200 }, + json: { result: [{ data: [{ country: 'US' }] }] }, + }); + + renderFilterValue(); + + await waitFor(() => { + expect(screen.getByTestId('mock-super-chart')).toBeInTheDocument(); + }); + + expect(screen.queryByRole('status')).not.toBeInTheDocument(); +}); + +test('renders error state when API call fails', async () => { + mockGetChartDataRequest.mockRejectedValue( + new Response(JSON.stringify({ message: 'Server Error' }), { status: 500 }), + ); + + renderFilterValue(); + + await waitFor(() => { + expect(screen.queryByRole('status')).not.toBeInTheDocument(); + }); +}); + +test('does not fetch data when filter has not been in view', () => { + renderFilterValue({ inView: false }); + + expect(mockGetChartDataRequest).not.toHaveBeenCalled(); +}); + +test('does not render loading spinner when filter has no data source', () => { + const filterWithoutDataSource = createMockFilter({ + targets: [{ column: { name: 'country' } }], + }); + mockGetChartDataRequest.mockReturnValue(new Promise(() => {})); + + renderFilterValue({ filter: filterWithoutDataSource }); + + expect(screen.queryByRole('status')).not.toBeInTheDocument(); + expect(screen.getByTestId('mock-super-chart')).toBeInTheDocument(); +}); + +test('skips data fetch when cascade parent filters have no values selected', () => { + // useFilterDependencies returns dependencies with a filter (from parent defaults), + // but dataMaskSelected has no extraFormData for the parent -- counts disagree, so + // the component skips the fetch. + mockUseFilterDependencies.mockReturnValue({ + filters: [{ col: 'region', op: 'IN', val: ['US'] }], + }); + + const childFilter = createMockFilter({ + id: 'NATIVE_FILTER-CHILD', + cascadeParentIds: ['NATIVE_FILTER-PARENT'], + }); + + const stateWithParent = { + nativeFilters: { + filters: { + 'NATIVE_FILTER-CHILD': childFilter, + 'NATIVE_FILTER-PARENT': createMockFilter({ + id: 'NATIVE_FILTER-PARENT', + }), + }, + filterSets: {}, + }, + }; + + renderFilterValue( + { + filter: childFilter, + dataMaskSelected: {}, + }, + stateWithParent, + ); + + expect(mockGetChartDataRequest).not.toHaveBeenCalled(); +}); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx index c23f2578718..c7b2e79fadd 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx @@ -26,7 +26,7 @@ import { useState, } from 'react'; -import { t } from '@apache-superset/core/translation'; +import { t } from '@apache-superset/core'; import { ChartDataResponseResult, Behavior, @@ -41,7 +41,8 @@ import { getClientErrorObject, isChartCustomization, } from '@superset-ui/core'; -import { styled } from '@apache-superset/core/theme'; +import { styled, SupersetTheme } from '@apache-superset/core/ui'; +import { useTheme } from '@emotion/react'; import { useDispatch, useSelector } from 'react-redux'; import { isEqual, isEqualWith } from 'lodash'; import { getChartDataRequest } from 'src/components/Chart/chartAction'; @@ -141,6 +142,7 @@ const FilterValue: FC = ({ clearAllTrigger, onClearAllComplete, }) => { + const theme = useTheme() as SupersetTheme; const { id, targets, filterType } = filter; const isCustomization = isChartCustomization(filter); const allowedTimeGrains = isCustomization @@ -487,6 +489,7 @@ const FilterValue: FC = ({ enableNoResults={metadata?.enableNoResults} isRefreshing={isRefreshing} hooks={hooks} + theme={theme} /> )} diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx index 3d3d5e4d77f..390fdc2b0fc 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx @@ -17,7 +17,8 @@ * under the License. */ import { FC, useMemo } from 'react'; -import { t } from '@apache-superset/core/translation'; +import { t, SupersetTheme } from '@apache-superset/core/ui'; +import { useTheme } from '@emotion/react'; import { Behavior, SetDataMaskHook, @@ -52,6 +53,7 @@ const DefaultValue: FC = ({ formData, enableNoResults, }) => { + const theme = useTheme() as SupersetTheme; const formFilter = form.getFieldValue('filters')?.[filterId]; const queriesData = formFilter?.defaultValueQueriesData; const chartType = formFilter?.filterType; @@ -81,6 +83,7 @@ const DefaultValue: FC = ({ validateMessage: isMissingRequiredValue && t('Value is required'), validateStatus: isMissingRequiredValue && 'error', }} + theme={theme} /> ); };