From 2c49d12060e628893acb51f218226b74b7e40553 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Mon, 6 Mar 2023 10:51:34 +0100 Subject: [PATCH 1/7] feat(dashboard): Don't show cross filters badge on incompatible charts --- .../FilterBar/CrossFilters/Vertical.tsx | 10 +- .../FilterBar/CrossFilters/selectors.ts | 46 +++++---- .../FilterControls/FilterControls.tsx | 10 +- .../nativeFilters/FilterBar/Horizontal.tsx | 9 +- .../components/nativeFilters/selectors.ts | 93 ++++++++++++------- 5 files changed, 95 insertions(+), 73 deletions(-) diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/Vertical.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/Vertical.tsx index 93fb649d686f6..907e73c39e930 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/Vertical.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/Vertical.tsx @@ -18,9 +18,9 @@ */ import React from 'react'; -import { DataMaskStateWithId } from '@superset-ui/core'; +import { DataMaskStateWithId, JsonObject } from '@superset-ui/core'; import { useSelector } from 'react-redux'; -import { DashboardInfo, DashboardLayout, RootState } from 'src/dashboard/types'; +import { DashboardLayout, RootState } from 'src/dashboard/types'; import crossFiltersSelector from './selectors'; import VerticalCollapse from './VerticalCollapse'; @@ -28,15 +28,15 @@ const CrossFiltersVertical = () => { const dataMask = useSelector( state => state.dataMask, ); - const dashboardInfo = useSelector( - state => state.dashboardInfo, + const chartConfiguration = useSelector( + state => state.dashboardInfo.metadata?.chart_configuration, ); const dashboardLayout = useSelector( state => state.dashboardLayout.present, ); const selectedCrossFilters = crossFiltersSelector({ dataMask, - dashboardInfo, + chartConfiguration, dashboardLayout, }); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/selectors.ts b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/selectors.ts index c0f45af5b3e6c..bf19ecabf9e2e 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/selectors.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/selectors.ts @@ -17,37 +17,35 @@ * under the License. */ -import { DataMaskStateWithId } from '@superset-ui/core'; -import { DashboardInfo, DashboardLayout } from 'src/dashboard/types'; -import { CrossFilterIndicator, selectChartCrossFilters } from '../../selectors'; +import { DataMaskStateWithId, isDefined, JsonObject } from '@superset-ui/core'; +import { DashboardLayout } from 'src/dashboard/types'; +import { CrossFilterIndicator, getCrossFilterIndicator } from '../../selectors'; export const crossFiltersSelector = (props: { dataMask: DataMaskStateWithId; - dashboardInfo: DashboardInfo; + chartConfiguration: JsonObject; dashboardLayout: DashboardLayout; }): CrossFilterIndicator[] => { - const { dataMask, dashboardInfo, dashboardLayout } = props; - const chartConfiguration = dashboardInfo.metadata?.chart_configuration; + const { dataMask, chartConfiguration, dashboardLayout } = props; const chartsIds = Object.keys(chartConfiguration); - const shouldFilterEmitters = true; - let selectedCrossFilters: CrossFilterIndicator[] = []; - - for (let i = 0; i < chartsIds.length; i += 1) { - const chartId = Number(chartsIds[i]); - const crossFilters = selectChartCrossFilters( - dataMask, - chartId, - dashboardLayout, - chartConfiguration, - shouldFilterEmitters, - ); - selectedCrossFilters = [ - ...selectedCrossFilters, - ...(crossFilters as CrossFilterIndicator[]), - ]; - } - return selectedCrossFilters; + return chartsIds + .map(chartId => { + const id = Number(chartId); + const filterIndicator = getCrossFilterIndicator( + id, + dataMask[id], + dashboardLayout, + ); + if ( + isDefined(filterIndicator.column) && + isDefined(filterIndicator.value) + ) { + return { ...filterIndicator, emitterId: id }; + } + return null; + }) + .filter(Boolean) as CrossFilterIndicator[]; }; export default crossFiltersSelector; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx index 589e7609e852a..b44591f4b1a9c 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx @@ -35,6 +35,7 @@ import { isFeatureEnabled, FeatureFlag, isNativeFilterWithDataMask, + JsonObject, } from '@superset-ui/core'; import { createHtmlPortalNode, @@ -47,7 +48,6 @@ import { useSelectFiltersInScope, } from 'src/dashboard/components/nativeFilters/state'; import { - DashboardInfo, DashboardLayout, FilterBarOrientation, RootState, @@ -87,8 +87,8 @@ const FilterControls: FC = ({ const dataMask = useSelector( state => state.dataMask, ); - const dashboardInfo = useSelector( - state => state.dashboardInfo, + const chartConfiguration = useSelector( + state => state.dashboardInfo.metadata?.chart_configuration, ); const dashboardLayout = useSelector( state => state.dashboardLayout.present, @@ -101,11 +101,11 @@ const FilterControls: FC = ({ isCrossFiltersEnabled ? crossFiltersSelector({ dataMask, - dashboardInfo, + chartConfiguration, dashboardLayout, }) : [], - [dashboardInfo, dashboardLayout, dataMask, isCrossFiltersEnabled], + [chartConfiguration, dashboardLayout, dataMask, isCrossFiltersEnabled], ); const { filterControlFactory, filtersWithValues } = useFilterControlFactory( dataMaskSelected, diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx index badff642955a4..24b2caa033b81 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx @@ -22,12 +22,13 @@ import { DataMaskStateWithId, FeatureFlag, isFeatureEnabled, + JsonObject, styled, t, } from '@superset-ui/core'; import Icons from 'src/components/Icons'; import Loading from 'src/components/Loading'; -import { DashboardInfo, DashboardLayout, RootState } from 'src/dashboard/types'; +import { DashboardLayout, RootState } from 'src/dashboard/types'; import { useSelector } from 'react-redux'; import FilterControls from './FilterControls/FilterControls'; import { getFilterBarTestId } from './utils'; @@ -107,8 +108,8 @@ const HorizontalFilterBar: React.FC = ({ const dataMask = useSelector( state => state.dataMask, ); - const dashboardInfo = useSelector( - state => state.dashboardInfo, + const chartConfiguration = useSelector( + state => state.dashboardInfo.metadata?.chart_configuration, ); const dashboardLayout = useSelector( state => state.dashboardLayout.present, @@ -119,7 +120,7 @@ const HorizontalFilterBar: React.FC = ({ const selectedCrossFilters = isCrossFiltersEnabled ? crossFiltersSelector({ dataMask, - dashboardInfo, + chartConfiguration, dashboardLayout, }) : []; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/selectors.ts b/superset-frontend/src/dashboard/components/nativeFilters/selectors.ts index 27845727aaf0f..5d4e980a40344 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/selectors.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/selectors.ts @@ -17,6 +17,7 @@ * under the License. */ import { + DataMask, DataMaskStateWithId, DataMaskType, ensureIsArray, @@ -32,7 +33,7 @@ import { import { TIME_FILTER_MAP } from 'src/explore/constants'; import { getChartIdsInFilterBoxScope } from 'src/dashboard/util/activeDashboardFilters'; import { ChartConfiguration } from 'src/dashboard/reducers/types'; -import { Layout } from 'src/dashboard/types'; +import { DashboardLayout, Layout } from 'src/dashboard/types'; import { areObjectsEqual } from 'src/reduxUtils'; export enum IndicatorStatus { @@ -61,7 +62,7 @@ type Filter = { datasourceId: string; }; -const extractLabel = (filter?: FilterState): string | null => { +export const extractLabel = (filter?: FilterState): string | null => { if (filter?.label && !filter?.label?.includes(undefined)) { return filter.label; } @@ -162,6 +163,36 @@ export type Indicator = { export type CrossFilterIndicator = Indicator & { emitterId: number }; +export const getCrossFilterIndicator = ( + chartId: number, + dataMask: DataMask, + dashboardLayout: DashboardLayout, +) => { + const filterState = dataMask?.filterState; + const filters = dataMask?.extraFormData?.filters; + const label = extractLabel(filterState); + const filtersState = filterState?.filters; + const column = + filters?.[0]?.col || (filtersState && Object.keys(filtersState)[0]); + + const dashboardLayoutItem = Object.values(dashboardLayout).find( + layoutItem => layoutItem?.meta?.chartId === chartId, + ); + const filterObject: Indicator = { + column, + name: + dashboardLayoutItem?.meta?.sliceNameOverride || + dashboardLayoutItem?.meta?.sliceName || + '', + path: [ + ...(dashboardLayoutItem?.parents ?? []), + dashboardLayoutItem?.id || '', + ], + value: label, + }; + return filterObject; +}; + const cachedIndicatorsForChart = {}; const cachedDashboardFilterDataForChart = {}; // inspects redux state to find what the filter indicators should be shown for a given chart @@ -233,17 +264,18 @@ const getStatus = ({ }): IndicatorStatus => { // a filter is only considered unset if it's value is null const hasValue = label !== null; - if (type === DataMaskType.CrossFilters && hasValue) { - return IndicatorStatus.CrossFilterApplied; - } + const APPLIED_STATUS = + type === DataMaskType.CrossFilters + ? IndicatorStatus.CrossFilterApplied + : IndicatorStatus.Applied; if (!column && hasValue) { // Filter without datasource - return IndicatorStatus.Applied; + return APPLIED_STATUS; } if (column && rejectedColumns?.has(column)) return IndicatorStatus.Incompatible; if (column && appliedColumns?.has(column) && hasValue) { - return IndicatorStatus.Applied; + return APPLIED_STATUS; } return IndicatorStatus.Unset; }; @@ -254,11 +286,12 @@ export const selectChartCrossFilters = ( chartId: number, dashboardLayout: Layout, chartConfiguration: ChartConfiguration = defaultChartConfig, + appliedColumns: Set, + rejectedColumns: Set, filterEmitter = false, ): Indicator[] | CrossFilterIndicator[] => { let crossFilterIndicators: any = []; if (isFeatureEnabled(FeatureFlag.DASHBOARD_CROSS_FILTERS)) { - const dashboardLayoutValues = Object.values(dashboardLayout); crossFilterIndicators = Object.values(chartConfiguration) .filter(chartConfig => { const inScope = @@ -272,34 +305,22 @@ export const selectChartCrossFilters = ( return false; }) .map(chartConfig => { - const filterState = dataMask[chartConfig.id]?.filterState; - const extraFormData = dataMask[chartConfig.id]?.extraFormData; - const label = extractLabel(filterState); - const filtersState = filterState?.filters; - const column = - extraFormData?.filters?.[0]?.col || - (filtersState && Object.keys(filtersState)[0]); - - const dashboardLayoutItem = dashboardLayoutValues.find( - layoutItem => layoutItem?.meta?.chartId === chartConfig.id, + const filterIndicator = getCrossFilterIndicator( + chartConfig.id, + dataMask[chartConfig.id], + dashboardLayout, ); - const filterObject: Indicator = { - column, - name: dashboardLayoutItem?.meta?.sliceName as string, - path: [ - ...(dashboardLayoutItem?.parents ?? []), - dashboardLayoutItem?.id || '', - ], - status: getStatus({ - label, - type: DataMaskType.CrossFilters, - }), - value: label, - }; - if (filterEmitter) { - (filterObject as CrossFilterIndicator).emitterId = chartId; - } - return filterObject; + const filterStatus = getStatus({ + label: filterIndicator.value, + column: filterIndicator.column + ? getColumnLabel(filterIndicator.column) + : undefined, + type: DataMaskType.CrossFilters, + appliedColumns, + rejectedColumns, + }); + + return { ...filterIndicator, status: filterStatus }; }) .filter(filter => filter.status === IndicatorStatus.CrossFilterApplied); } @@ -369,6 +390,8 @@ export const selectNativeIndicatorsForChart = ( chartId, dashboardLayout, chartConfiguration, + appliedColumns, + rejectedColumns, ); } const indicators = crossFilterIndicators.concat(nativeFilterIndicators); From 00f37d500ad459eef3a678d9c256f168a6b9e862 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Mon, 6 Mar 2023 18:04:46 +0100 Subject: [PATCH 2/7] Restyle filters badge tooltip --- .../FiltersBadge/DetailsPanel/index.tsx | 209 ++++-------------- .../FiltersBadge/FilterIndicator/index.tsx | 37 ++-- .../FiltersBadge/FiltersBadge.test.tsx | 63 ------ .../components/FiltersBadge/Styles.tsx | 109 ++++----- .../components/FiltersBadge/index.tsx | 47 +--- .../components/SliceHeader/index.tsx | 30 +-- 6 files changed, 125 insertions(+), 370 deletions(-) diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx b/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx index 022dab796875f..0e69b78a2c800 100644 --- a/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx +++ b/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx @@ -19,31 +19,21 @@ import React, { useEffect, useState } from 'react'; import { useSelector } from 'react-redux'; import { Global, css } from '@emotion/react'; -import { t, useTheme } from '@superset-ui/core'; +import { t } from '@superset-ui/core'; import Popover from 'src/components/Popover'; -import Collapse from 'src/components/Collapse'; -import Icons from 'src/components/Icons'; import { - Indent, - Panel, - Reset, - Title, + FiltersContainer, + FiltersDetailsContainer, + Separator, + SectionName, } from 'src/dashboard/components/FiltersBadge/Styles'; import { Indicator } from 'src/dashboard/components/nativeFilters/selectors'; import FilterIndicator from 'src/dashboard/components/FiltersBadge/FilterIndicator'; import { RootState } from 'src/dashboard/types'; -const iconReset = css` - span { - line-height: 0; - } -`; - export interface DetailsPanelProps { appliedCrossFilterIndicators: Indicator[]; appliedIndicators: Indicator[]; - incompatibleIndicators: Indicator[]; - unsetIndicators: Indicator[]; onHighlightFilterSource: (path: string[]) => void; children: JSX.Element; } @@ -51,13 +41,10 @@ export interface DetailsPanelProps { const DetailsPanelPopover = ({ appliedCrossFilterIndicators = [], appliedIndicators = [], - incompatibleIndicators = [], - unsetIndicators = [], onHighlightFilterSource, children, }: DetailsPanelProps) => { const [visible, setVisible] = useState(false); - const theme = useTheme(); const activeTabs = useSelector( state => state.dashboardState?.activeTabs, ); @@ -76,57 +63,22 @@ const DetailsPanelPopover = ({ setVisible(false); }, [activeTabs]); - const getDefaultActivePanel = () => { - const result = []; - if (appliedCrossFilterIndicators.length) { - result.push('appliedCrossFilters'); - } - if (appliedIndicators.length) { - result.push('applied'); - } - if (incompatibleIndicators.length) { - result.push('incompatible'); - } - if (result.length) { - return result; - } - return ['unset']; - }; - - const [activePanels, setActivePanels] = useState(() => [ - ...getDefaultActivePanel(), - ]); - function handlePopoverStatus(isOpen: boolean) { setVisible(isOpen); - // every time the popover opens, make sure the most relevant panel is active - if (isOpen) { - setActivePanels(getDefaultActivePanel()); - } - } - - function handleActivePanelChange(panels: string | string[]) { - // need to convert to an array so that handlePopoverStatus will work - if (typeof panels === 'string') { - setActivePanels([panels]); - } else { - setActivePanels(panels); - } } const indicatorKey = (indicator: Indicator): string => `${indicator.column} - ${indicator.name}`; const content = ( - + css` .filterStatusPopover { .ant-popover-inner { background-color: ${theme.colors.grayscale.dark2}cc; .ant-popover-inner-content { - padding-top: 0; - padding-bottom: 0; + padding: ${theme.gridUnit * 2}px; } } &.ant-popover-placement-bottom, @@ -168,110 +120,47 @@ const DetailsPanelPopover = ({ } `} /> - - - {appliedCrossFilterIndicators.length ? ( - - - {t( - 'Applied Cross Filters (%d)', - appliedCrossFilterIndicators.length, - )} - - } - > - - {appliedCrossFilterIndicators.map(indicator => ( - - ))} - - - ) : null} - {appliedIndicators.length ? ( - - {' '} - {t('Applied Filters (%d)', appliedIndicators.length)} - - } - > - - {appliedIndicators.map(indicator => ( - - ))} - - - ) : null} - {incompatibleIndicators.length ? ( - - {' '} - {t( - 'Incompatible Filters (%d)', - incompatibleIndicators.length, - )} - - } - > - - {incompatibleIndicators.map(indicator => ( - - ))} - - - ) : null} - {unsetIndicators.length ? ( - - {' '} - {t('Unset Filters (%d)', unsetIndicators.length)} - - } - disabled={!unsetIndicators.length} - > - - {unsetIndicators.map(indicator => ( - - ))} - - - ) : null} - - - +
+ {appliedCrossFilterIndicators.length ? ( +
+ + {t( + 'Applied Cross Filters (%d)', + appliedCrossFilterIndicators.length, + )} + + + {appliedCrossFilterIndicators.map(indicator => ( + + ))} + +
+ ) : null} + {appliedCrossFilterIndicators.length && appliedIndicators.length ? ( + + ) : null} + {appliedIndicators.length ? ( +
+ + {t('Applied filters (%d)', appliedIndicators.length)} + + + {appliedIndicators.map(indicator => ( + + ))} + +
+ ) : null} +
+ ); return ( diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/FilterIndicator/index.tsx b/superset-frontend/src/dashboard/components/FiltersBadge/FilterIndicator/index.tsx index 2954474e64eac..9713feb0ddb45 100644 --- a/superset-frontend/src/dashboard/components/FiltersBadge/FilterIndicator/index.tsx +++ b/superset-frontend/src/dashboard/components/FiltersBadge/FilterIndicator/index.tsx @@ -22,31 +22,31 @@ import { css } from '@superset-ui/core'; import Icons from 'src/components/Icons'; import { getFilterValueForDisplay } from 'src/dashboard/components/nativeFilters/FilterBar/FilterSets/utils'; import { - FilterIndicatorText, FilterValue, - Item, - ItemIcon, - Title, + FilterItem, + FilterName, } from 'src/dashboard/components/FiltersBadge/Styles'; import { Indicator } from 'src/dashboard/components/nativeFilters/selectors'; export interface IndicatorProps { indicator: Indicator; onClick?: (path: string[]) => void; - text?: string; } const FilterIndicator: FC = ({ indicator: { column, name, value, path = [] }, - onClick = () => {}, - text, + onClick, }) => { const resultValue = getFilterValueForDisplay(value); return ( - <> - onClick([...path, `LABEL-${column}`])}> - - <ItemIcon> + <FilterItem + onClick={ + onClick ? () => onClick([...path, `LABEL-${column}`]) : undefined + } + > + <FilterName> + {onClick && ( + <i> <Icons.SearchOutlined iconSize="m" css={css` @@ -55,14 +55,13 @@ const FilterIndicator: FC<IndicatorProps> = ({ } `} /> - </ItemIcon> - {name} - {resultValue ? ': ' : ''} - - {resultValue} - - {text && {text}} - + + )} + {name} + {resultValue ? ': ' : ''} + + {resultValue} + ); }; diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/FiltersBadge.test.tsx b/superset-frontend/src/dashboard/components/FiltersBadge/FiltersBadge.test.tsx index 2a1f7bcfb03bc..c9e6c9e1fce99 100644 --- a/superset-frontend/src/dashboard/components/FiltersBadge/FiltersBadge.test.tsx +++ b/superset-frontend/src/dashboard/components/FiltersBadge/FiltersBadge.test.tsx @@ -36,7 +36,6 @@ import { import { sliceId } from 'spec/fixtures/mockChartQueries'; import { dashboardFilters } from 'spec/fixtures/mockDashboardFilters'; import { dashboardWithFilter } from 'spec/fixtures/mockDashboardLayout'; -import Icons from 'src/components/Icons'; import { FeatureFlag } from 'src/featureFlags'; const defaultStore = getMockStoreWithFilters(); @@ -111,36 +110,6 @@ describe('FiltersBadge', () => { ); expect(wrapper.find('WarningFilled')).not.toExist(); }); - - it("shows a warning when there's a rejected filter", () => { - const store = getMockStoreWithFilters(); - // start with basic dashboard state, dispatch an event to simulate query completion - store.dispatch({ - type: CHART_UPDATE_SUCCEEDED, - key: sliceId, - queriesResponse: [ - { - status: 'success', - applied_filters: [], - rejected_filters: [ - { column: 'region', reason: 'not_in_datasource' }, - ], - }, - ], - dashboardFilters, - }); - store.dispatch({ type: CHART_RENDERING_SUCCEEDED, key: sliceId }); - const wrapper = setup(store); - expect(wrapper.find('DetailsPanelPopover')).toExist(); - expect(wrapper.find('[data-test="applied-filter-count"]')).toHaveText( - '0', - ); - expect( - wrapper.find('[data-test="incompatible-filter-count"]'), - ).toHaveText('1'); - // to look at the shape of the wrapper use: - expect(wrapper.find(Icons.AlertSolid)).toExist(); - }); }); describe('for native filters', () => { @@ -189,37 +158,5 @@ describe('FiltersBadge', () => { ); expect(wrapper.find('WarningFilled')).not.toExist(); }); - - it("shows a warning when there's a rejected filter", () => { - // @ts-ignore - global.featureFlags = { - [FeatureFlag.DASHBOARD_NATIVE_FILTERS]: true, - }; - const store = getMockStoreWithNativeFilters(); - // start with basic dashboard state, dispatch an event to simulate query completion - store.dispatch({ - type: CHART_UPDATE_SUCCEEDED, - key: sliceId, - queriesResponse: [ - { - status: 'success', - applied_filters: [], - rejected_filters: [ - { column: 'region', reason: 'not_in_datasource' }, - ], - }, - ], - }); - store.dispatch({ type: CHART_RENDERING_SUCCEEDED, key: sliceId }); - const wrapper = setup(store); - expect(wrapper.find('DetailsPanelPopover')).toExist(); - expect(wrapper.find('[data-test="applied-filter-count"]')).toHaveText( - '0', - ); - expect( - wrapper.find('[data-test="incompatible-filter-count"]'), - ).toHaveText('1'); - expect(wrapper.find(Icons.AlertSolid)).toExist(); - }); }); }); diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx b/superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx index c80e0ec0cb734..410be36e1f772 100644 --- a/superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx +++ b/superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx @@ -20,7 +20,7 @@ import { css, styled } from '@superset-ui/core'; export const Pill = styled.div` ${({ theme }) => css` - display: inline-block; + display: flex; color: ${theme.colors.grayscale.light5}; background: ${theme.colors.grayscale.base}; border-radius: 1em; @@ -36,7 +36,6 @@ export const Pill = styled.div` svg { position: relative; - top: -2px; color: ${theme.colors.grayscale.light5}; width: 1em; height: 1em; @@ -55,64 +54,32 @@ export const Pill = styled.div` background: ${theme.colors.primary.dark1}; } } - - &.has-incompatible-filters { - color: ${theme.colors.grayscale.dark2}; - background: ${theme.colors.alert.base}; - &:hover { - background: ${theme.colors.alert.dark1}; - } - svg { - color: ${theme.colors.grayscale.dark2}; - } - } - - &.filters-inactive { - color: ${theme.colors.grayscale.light5}; - background: ${theme.colors.grayscale.light1}; - padding: ${theme.gridUnit}px; - text-align: center; - height: 22px; - width: 22px; - - &:hover { - background: ${theme.colors.grayscale.base}; - } - } `} `; -export interface TitleProps { - bold?: boolean; - color?: string; -} - -export const Title = styled.span` - position: relative; - margin-right: ${({ theme }) => theme.gridUnit}px; - font-weight: ${({ bold, theme }) => { - if (bold) return theme.typography.weights.bold; - return 'auto'; - }}; - color: ${({ color, theme }) => color || theme.colors.grayscale.light5}; - display: flex; - align-items: center; - & > * { - margin-right: ${({ theme }) => theme.gridUnit}px; - } +export const SectionName = styled.span` + ${({ theme }) => css` + font-weight: ${theme.typography.weights.bold}; + // & > * { + // margin-right: ${theme.gridUnit}px; + // } + `} `; - -export const ItemIcon = styled.i` - position: absolute; - top: 50%; - transform: translateY(-50%); - left: -${({ theme }) => theme.gridUnit * 5}px; +export const FilterName = styled.span` + ${({ theme }) => css` + padding-right: ${theme.gridUnit}px; + display: flex; + align-items: center; + font-style: italic; + & > * { + margin-right: ${theme.gridUnit}px; + } + `} `; -export const Item = styled.button` +export const FilterItem = styled.button` cursor: pointer; display: flex; - flex-wrap: wrap; text-align: left; padding: 0; border: none; @@ -134,34 +101,36 @@ export const Item = styled.button` } `; -export const Reset = styled.div` - margin: 0 -${({ theme }) => theme.gridUnit * 4}px; +export const FiltersContainer = styled.div` + ${({ theme }) => css` + margin-top: ${theme.gridUnit}px; + &:not(:last-child) { + padding-bottom: ${theme.gridUnit * 3}px; + } + `} `; -export const Indent = styled.div` - padding-left: ${({ theme }) => theme.gridUnit * 6}px; - margin: -${({ theme }) => theme.gridUnit * 3}px 0; -`; +export const FiltersDetailsContainer = styled.div` + ${({ theme }) => css` + min-width: 200px; + max-width: 300px; + overflow-x: hidden; -export const Panel = styled.div` - min-width: 200px; - max-width: 300px; - overflow-x: hidden; + color: ${theme.colors.grayscale.light5}; + `} `; export const FilterValue = styled.div` max-width: 100%; flex-grow: 1; overflow: auto; - color: ${({ theme }) => theme.colors.grayscale.light5}; `; -export const FilterIndicatorText = styled.div` - ${({ theme }) => ` - padding-top: ${theme.gridUnit * 3}px; - max-width: 100%; - flex-grow: 1; - overflow: auto; - color: ${theme.colors.grayscale.light5}; +export const Separator = styled.div` + ${({ theme }) => css` + width: 100%; + height: 2px; + background-color: ${theme.colors.grayscale.light1}; + margin: ${theme.gridUnit * 4}px 0; `} `; diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/index.tsx b/superset-frontend/src/dashboard/components/FiltersBadge/index.tsx index fb0718bf54cb4..655a97c7d83b3 100644 --- a/superset-frontend/src/dashboard/components/FiltersBadge/index.tsx +++ b/superset-frontend/src/dashboard/components/FiltersBadge/index.tsx @@ -211,66 +211,27 @@ export const FiltersBadge = ({ chartId }: FiltersBadgeProps) => { ), [indicators], ); - const unsetIndicators = useMemo( - () => - indicators.filter( - indicator => indicator.status === IndicatorStatus.Unset, - ), - [indicators], - ); - const incompatibleIndicators = useMemo( - () => - indicators.filter( - indicator => indicator.status === IndicatorStatus.Incompatible, - ), - [indicators], - ); - if ( - !appliedCrossFilterIndicators.length && - !appliedIndicators.length && - !incompatibleIndicators.length && - !unsetIndicators.length - ) { + if (!appliedCrossFilterIndicators.length && !appliedIndicators.length) { return null; } - const isInactive = - !appliedCrossFilterIndicators.length && - !appliedIndicators.length && - !incompatibleIndicators.length; - return ( - {!isInactive && ( - - {appliedIndicators.length + appliedCrossFilterIndicators.length} - - )} - {incompatibleIndicators.length ? ( - <> - {' '} - - - {incompatibleIndicators.length} - - - ) : null} + + {appliedIndicators.length + appliedCrossFilterIndicators.length} + ); diff --git a/superset-frontend/src/dashboard/components/SliceHeader/index.tsx b/superset-frontend/src/dashboard/components/SliceHeader/index.tsx index a928933b54a27..41b6a7dbe76d3 100644 --- a/superset-frontend/src/dashboard/components/SliceHeader/index.tsx +++ b/superset-frontend/src/dashboard/components/SliceHeader/index.tsx @@ -21,11 +21,10 @@ import React, { ReactNode, useContext, useEffect, - useMemo, useRef, useState, } from 'react'; -import { css, styled, t } from '@superset-ui/core'; +import { css, styled, SupersetTheme, t } from '@superset-ui/core'; import { useUiConfig } from 'src/components/UiConfigContext'; import { Tooltip } from 'src/components/Tooltip'; import { useDispatch, useSelector } from 'react-redux'; @@ -36,10 +35,10 @@ import SliceHeaderControls, { import FiltersBadge from 'src/dashboard/components/FiltersBadge'; import Icons from 'src/components/Icons'; import { RootState } from 'src/dashboard/types'; -import FilterIndicator from 'src/dashboard/components/FiltersBadge/FilterIndicator'; import { getSliceHeaderTooltip } from 'src/dashboard/util/getSliceHeaderTooltip'; import { DashboardPageIdContext } from 'src/dashboard/containers/DashboardPage'; import { clearDataMask } from 'src/dataMask/actions'; +import { getFilterValueForDisplay } from '../nativeFilters/FilterBar/FilterSets/utils'; type SliceHeaderProps = SliceHeaderControlsProps & { innerRef?: string; @@ -173,14 +172,6 @@ const SliceHeader: FC = ({ ({ dashboardInfo }) => dashboardInfo.crossFiltersEnabled, ); - const indicator = useMemo( - () => ({ - value: crossFilterValue, - name: t('Emitted values'), - }), - [crossFilterValue], - ); - const canExplore = !editMode && supersetCanExplore; useEffect(() => { @@ -251,10 +242,19 @@ const SliceHeader: FC = ({ +
+ {t('Emitted values: ')} + {getFilterValueForDisplay(crossFilterValue)} +
+ css` + margin-top: ${theme.gridUnit * 2}px; + ` + } + > + {t('Click to clear emitted filters')} +
+
} > Date: Mon, 6 Mar 2023 18:26:12 +0100 Subject: [PATCH 3/7] Make the popover open on hover --- .../DetailsPanel/DetailsPanel.test.tsx | 80 ++----------------- .../FiltersBadge/DetailsPanel/index.tsx | 4 +- 2 files changed, 10 insertions(+), 74 deletions(-) diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/DetailsPanel.test.tsx b/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/DetailsPanel.test.tsx index 3c3e25ee56a8e..ef4b49d218808 100644 --- a/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/DetailsPanel.test.tsx +++ b/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/DetailsPanel.test.tsx @@ -86,7 +86,7 @@ const createProps = () => ({ onHighlightFilterSource: jest.fn(), }); -test('Should render "appliedCrossFilterIndicators"', () => { +test('Should render "appliedCrossFilterIndicators"', async () => { const props = createProps(); props.appliedIndicators = []; props.incompatibleIndicators = []; @@ -99,8 +99,10 @@ test('Should render "appliedCrossFilterIndicators"', () => { { useRedux: true }, ); - userEvent.click(screen.getByTestId('details-panel-content')); - expect(screen.getByText('Applied Cross Filters (1)')).toBeInTheDocument(); + userEvent.hover(screen.getByTestId('details-panel-content')); + expect( + await screen.findByText('Applied cross-filters (1)'), + ).toBeInTheDocument(); expect( screen.getByRole('button', { name: 'Clinical Stage' }), ).toBeInTheDocument(); @@ -118,7 +120,7 @@ test('Should render "appliedCrossFilterIndicators"', () => { ]); }); -test('Should render "appliedIndicators"', () => { +test('Should render "appliedIndicators"', async () => { const props = createProps(); props.appliedCrossFilterIndicators = []; props.incompatibleIndicators = []; @@ -131,8 +133,8 @@ test('Should render "appliedIndicators"', () => { { useRedux: true }, ); - userEvent.click(screen.getByTestId('details-panel-content')); - expect(screen.getByText('Applied Filters (1)')).toBeInTheDocument(); + userEvent.hover(screen.getByTestId('details-panel-content')); + expect(await screen.findByText('Applied filters (1)')).toBeInTheDocument(); expect(screen.getByRole('button', { name: 'Country' })).toBeInTheDocument(); expect(props.onHighlightFilterSource).toBeCalledTimes(0); @@ -148,72 +150,6 @@ test('Should render "appliedIndicators"', () => { ]); }); -test('Should render "incompatibleIndicators"', () => { - const props = createProps(); - props.appliedCrossFilterIndicators = []; - props.appliedIndicators = []; - props.unsetIndicators = []; - - render( - -
Content
- , - { useRedux: true }, - ); - - userEvent.click(screen.getByTestId('details-panel-content')); - expect(screen.getByText('Incompatible Filters (1)')).toBeInTheDocument(); - expect( - screen.getByRole('button', { name: 'Vaccine Approach Copy' }), - ).toBeInTheDocument(); - - expect(props.onHighlightFilterSource).toBeCalledTimes(0); - userEvent.click( - screen.getByRole('button', { name: 'Vaccine Approach Copy' }), - ); - expect(props.onHighlightFilterSource).toBeCalledTimes(1); - expect(props.onHighlightFilterSource).toBeCalledWith([ - 'ROOT_ID', - 'TABS-wUKya7eQ0Zz', - 'TAB-BCIJF4NvgQq', - 'ROW-xSeNAspgww', - 'CHART-eirDduqb1Aa', - 'LABEL-product_category_copy', - ]); -}); - -test('Should render "unsetIndicators"', () => { - const props = createProps(); - props.appliedCrossFilterIndicators = []; - props.appliedIndicators = []; - props.incompatibleIndicators = []; - - render( - -
Content
- , - { useRedux: true }, - ); - - userEvent.click(screen.getByTestId('details-panel-content')); - expect(screen.getByText('Unset Filters (1)')).toBeInTheDocument(); - expect( - screen.getByRole('button', { name: 'Vaccine Approach' }), - ).toBeInTheDocument(); - - expect(props.onHighlightFilterSource).toBeCalledTimes(0); - userEvent.click(screen.getByRole('button', { name: 'Vaccine Approach' })); - expect(props.onHighlightFilterSource).toBeCalledTimes(1); - expect(props.onHighlightFilterSource).toBeCalledWith([ - 'ROOT_ID', - 'TABS-wUKya7eQ0Z', - 'TAB-BCIJF4NvgQ', - 'ROW-xSeNAspgw', - 'CHART-eirDduqb1A', - 'LABEL-product_category', - ]); -}); - test('Should render empty', () => { const props = createProps(); props.appliedCrossFilterIndicators = []; diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx b/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx index 0e69b78a2c800..53d20cd1f894c 100644 --- a/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx +++ b/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx @@ -125,7 +125,7 @@ const DetailsPanelPopover = ({
{t( - 'Applied Cross Filters (%d)', + 'Applied cross-filters (%d)', appliedCrossFilterIndicators.length, )} @@ -170,7 +170,7 @@ const DetailsPanelPopover = ({ visible={visible} onVisibleChange={handlePopoverStatus} placement="bottomRight" - trigger="click" + trigger="hover" > {children} From 69a4226b9e7ca24ca865046aa6420f7e64b6f727 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Thu, 9 Mar 2023 16:15:58 +0100 Subject: [PATCH 4/7] Update tooltip wrapping logic --- .../FiltersBadge/FilterIndicator/index.tsx | 36 ++++++++++--------- .../components/FiltersBadge/Styles.tsx | 4 +-- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/FilterIndicator/index.tsx b/superset-frontend/src/dashboard/components/FiltersBadge/FilterIndicator/index.tsx index 9713feb0ddb45..ed09e1679078b 100644 --- a/superset-frontend/src/dashboard/components/FiltersBadge/FilterIndicator/index.tsx +++ b/superset-frontend/src/dashboard/components/FiltersBadge/FilterIndicator/index.tsx @@ -44,23 +44,25 @@ const FilterIndicator: FC = ({ onClick ? () => onClick([...path, `LABEL-${column}`]) : undefined } > - - {onClick && ( - - - - )} - {name} - {resultValue ? ': ' : ''} - - {resultValue} + {onClick && ( + + + + )} +
+ + {name} + {resultValue ? ': ' : ''} + + {resultValue} +
); }; diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx b/superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx index 410be36e1f772..c482e968dbe2a 100644 --- a/superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx +++ b/superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx @@ -68,8 +68,6 @@ export const SectionName = styled.span` export const FilterName = styled.span` ${({ theme }) => css` padding-right: ${theme.gridUnit}px; - display: flex; - align-items: center; font-style: italic; & > * { margin-right: ${theme.gridUnit}px; @@ -120,7 +118,7 @@ export const FiltersDetailsContainer = styled.div` `} `; -export const FilterValue = styled.div` +export const FilterValue = styled.span` max-width: 100%; flex-grow: 1; overflow: auto; From 10d7c417bac3cdf729d0824f578ab95fe960977b Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Thu, 9 Mar 2023 18:40:19 +0100 Subject: [PATCH 5/7] Fix cypress test --- .../cypress/integration/dashboard/nativeFilters.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/cypress-base/cypress/integration/dashboard/nativeFilters.test.ts b/superset-frontend/cypress-base/cypress/integration/dashboard/nativeFilters.test.ts index 7e1d3d5371bb5..44de92c9c7c9e 100644 --- a/superset-frontend/cypress-base/cypress/integration/dashboard/nativeFilters.test.ts +++ b/superset-frontend/cypress-base/cypress/integration/dashboard/nativeFilters.test.ts @@ -379,7 +379,7 @@ describe('Horizontal FilterBar', () => { ]); setFilterBarOrientation('horizontal'); cy.getBySel('slice-header').within(() => { - cy.get('.filter-counts').click(); + cy.get('.filter-counts').trigger('mouseover'); }); cy.get('.filterStatusPopover').contains('test_8').click(); cy.getBySel('dropdown-content').should('be.visible'); From 570374800f19db4b13ae5cd8007fd14ef5d9cbdf Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Fri, 10 Mar 2023 13:09:49 +0100 Subject: [PATCH 6/7] Fix test --- .../cypress/integration/dashboard/nativeFilters.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/superset-frontend/cypress-base/cypress/integration/dashboard/nativeFilters.test.ts b/superset-frontend/cypress-base/cypress/integration/dashboard/nativeFilters.test.ts index 44de92c9c7c9e..1e119ae775c21 100644 --- a/superset-frontend/cypress-base/cypress/integration/dashboard/nativeFilters.test.ts +++ b/superset-frontend/cypress-base/cypress/integration/dashboard/nativeFilters.test.ts @@ -378,10 +378,13 @@ describe('Horizontal FilterBar', () => { { name: 'test_12', column: 'year', datasetId: 2 }, ]); setFilterBarOrientation('horizontal'); + openMoreFilters(); + applyNativeFilterValueWithIndex(8, testItems.filterDefaultValue); + cy.get(nativeFilters.applyFilter).click({ force: true }); cy.getBySel('slice-header').within(() => { cy.get('.filter-counts').trigger('mouseover'); }); - cy.get('.filterStatusPopover').contains('test_8').click(); + cy.get('.filterStatusPopover').contains('test_9').click(); cy.getBySel('dropdown-content').should('be.visible'); cy.get('.ant-select-focused').should('be.visible'); }); From 04a28251ae44d30e83ae2e280ee114632e560981 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Fri, 10 Mar 2023 17:38:34 +0100 Subject: [PATCH 7/7] Address comments --- .../src/dashboard/components/FiltersBadge/Styles.tsx | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx b/superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx index c482e968dbe2a..0018a007ba9e6 100644 --- a/superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx +++ b/superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx @@ -60,9 +60,6 @@ export const Pill = styled.div` export const SectionName = styled.span` ${({ theme }) => css` font-weight: ${theme.typography.weights.bold}; - // & > * { - // margin-right: ${theme.gridUnit}px; - // } `} `; export const FilterName = styled.span` @@ -127,7 +124,7 @@ export const FilterValue = styled.span` export const Separator = styled.div` ${({ theme }) => css` width: 100%; - height: 2px; + height: 1px; background-color: ${theme.colors.grayscale.light1}; margin: ${theme.gridUnit * 4}px 0; `}