Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
const dashboardLayout = useSelector<RootState, DashboardLayout>(
state => state.dashboardLayout.present,
);
const nativeFilters =
useSelector<RootState, Filters>(state => state.nativeFilters?.filters) ??
{};
const nativeFilters = useSelector<RootState, Filters>(
state => state.nativeFilters?.filters,
);
const directPathToChild = useSelector<RootState, string[]>(
state => state.dashboardState.directPathToChild,
);
Expand All @@ -68,7 +68,7 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
}, [getLeafComponentIdFromPath(directPathToChild)]);

// recalculate charts and tabs in scopes of native filters only when a scope or dashboard layout changes
const filterScopes = Object.values(nativeFilters).map(filter => ({
const filterScopes = Object.values(nativeFilters ?? {}).map(filter => ({
id: filter.id,
scope: filter.scope,
}));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/
import { useSelector } from 'react-redux';
import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags';
import { useEffect, useState } from 'react';
import { useCallback, useEffect, useState } from 'react';
import { URL_PARAMS } from 'src/constants';
import { getUrlParam } from 'src/utils/urlUtils';
import { RootState } from 'src/dashboard/types';
Expand Down Expand Up @@ -64,9 +64,12 @@ export const useNativeFilters = () => {
)
);

const toggleDashboardFiltersOpen = (visible?: boolean) => {
setDashboardFiltersOpen(visible ?? !dashboardFiltersOpen);
};
const toggleDashboardFiltersOpen = useCallback(
(visible?: boolean) => {
setDashboardFiltersOpen(visible ?? !dashboardFiltersOpen);
},
[dashboardFiltersOpen],
);

useEffect(() => {
if (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,5 +225,4 @@ const CascadePopover: React.FC<CascadePopoverProps> = ({
</Popover>
);
};

export default CascadePopover;
export default React.memo(CascadePopover);
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import React from 'react';
import React, { useMemo } from 'react';
import { styled } from '@superset-ui/core';
import { Form, FormItem } from 'src/components/Form';
import FilterValue from './FilterValue';
Expand Down Expand Up @@ -67,17 +67,22 @@ const FilterControl: React.FC<FilterProps> = ({
filter.dataMask?.filterState,
);

const label = useMemo(
() => (
<StyledFilterControlTitleBox>
<StyledFilterControlTitle data-test="filter-control-name">
{name}
</StyledFilterControlTitle>
<StyledIcon data-test="filter-icon">{icon}</StyledIcon>
</StyledFilterControlTitleBox>
),
[icon, name],
);

return (
<StyledFilterControlContainer layout="vertical">
<FormItem
label={
<StyledFilterControlTitleBox>
<StyledFilterControlTitle data-test="filter-control-name">
{name}
</StyledFilterControlTitle>
<StyledIcon data-test="filter-icon">{icon}</StyledIcon>
</StyledFilterControlTitleBox>
}
label={label}
required={filter?.controlValues?.enableEmptyFilter}
validateStatus={isMissingRequiredValue ? 'error' : undefined}
>
Expand All @@ -92,5 +97,4 @@ const FilterControl: React.FC<FilterProps> = ({
</StyledFilterControlContainer>
);
};

export default FilterControl;
export default React.memo(FilterControl);
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import React, { FC, useMemo, useState } from 'react';
import React, { FC, useCallback, useMemo, useState } from 'react';
import { css } from '@emotion/react';
import { DataMask, styled, t } from '@superset-ui/core';
import {
Expand Down Expand Up @@ -55,8 +55,8 @@ const FilterControls: FC<FilterControlsProps> = ({
}) => {
const [visiblePopoverId, setVisiblePopoverId] = useState<string | null>(null);
const filters = useFilters();
const filterValues = Object.values<Filter>(filters);
const portalNodes = React.useMemo(() => {
const filterValues = useMemo(() => Object.values<Filter>(filters), [filters]);
const portalNodes = useMemo(() => {
const nodes = new Array(filterValues.length);
for (let i = 0; i < filterValues.length; i += 1) {
nodes[i] = createHtmlPortalNode();
Expand All @@ -70,8 +70,7 @@ const FilterControls: FC<FilterControlsProps> = ({
dataMask: dataMaskSelected[filter.id],
}));
return buildCascadeFiltersTree(filtersWithValue);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [JSON.stringify(filterValues), dataMaskSelected]);
}, [filterValues, dataMaskSelected]);
const cascadeFilterIds = new Set(cascadeFilters.map(item => item.id));

const [filtersInScope, filtersOutOfScope] = useSelectFiltersInScope(
Expand All @@ -80,26 +79,36 @@ const FilterControls: FC<FilterControlsProps> = ({
const dashboardHasTabs = useDashboardHasTabs();
const showCollapsePanel = dashboardHasTabs && cascadeFilters.length > 0;

const cascadePopoverFactory = useCallback(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be useMemo here? Why are you preserving the instance of the function instead of its result?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a function, because the result depends on index which we get from portalNodes.map. Here the result will be memoized for each index value

index => (
<CascadePopover
data-test="cascade-filters-control"
key={cascadeFilters[index].id}
dataMaskSelected={dataMaskSelected}
visible={visiblePopoverId === cascadeFilters[index].id}
onVisibleChange={visible =>
setVisiblePopoverId(visible ? cascadeFilters[index].id : null)
}
filter={cascadeFilters[index]}
onFilterSelectionChange={onFilterSelectionChange}
directPathToChild={directPathToChild}
inView={false}
/>
),
[
cascadeFilters,
JSON.stringify(dataMaskSelected),
directPathToChild,
onFilterSelectionChange,
visiblePopoverId,
],
);
return (
<Wrapper>
{portalNodes
.filter((node, index) => cascadeFilterIds.has(filterValues[index].id))
.map((node, index) => (
<InPortal node={node}>
<CascadePopover
data-test="cascade-filters-control"
key={cascadeFilters[index].id}
dataMaskSelected={dataMaskSelected}
visible={visiblePopoverId === cascadeFilters[index].id}
onVisibleChange={visible =>
setVisiblePopoverId(visible ? cascadeFilters[index].id : null)
}
filter={cascadeFilters[index]}
onFilterSelectionChange={onFilterSelectionChange}
directPathToChild={directPathToChild}
inView={false}
/>
</InPortal>
<InPortal node={node}>{cascadePopoverFactory(index)}</InPortal>
))}
{filtersInScope.map(filter => {
const index = filterValues.findIndex(f => f.id === filter.id);
Expand Down Expand Up @@ -150,4 +159,4 @@ const FilterControls: FC<FilterControlsProps> = ({
);
};

export default FilterControls;
export default React.memo(FilterControls);
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,13 @@
* specific language governing permissions and limitations
* under the License.
*/
import React, { useEffect, useRef, useState } from 'react';
import React, {
useCallback,
useEffect,
useMemo,
useRef,
useState,
} from 'react';
import {
QueryFormData,
SuperChart,
Expand Down Expand Up @@ -53,6 +59,9 @@ const StyledDiv = styled.div`
}
`;

const queriesDataPlaceholder = [{ data: [{}] }];
const behaviors = [Behavior.NATIVE_FILTER];

const FilterValue: React.FC<FilterProps> = ({
dataMaskSelected,
filter,
Expand Down Expand Up @@ -193,11 +202,36 @@ const FilterValue: React.FC<FilterProps> = ({
return undefined;
}, [inputRef, directPathToChild, filter.id]);

const setDataMask = (dataMask: DataMask) =>
onFilterSelectionChange(filter, dataMask);
const setDataMask = useCallback(
(dataMask: DataMask) => onFilterSelectionChange(filter, dataMask),
[filter, onFilterSelectionChange],
);

const setFocusedFilter = useCallback(
() => dispatchFocusAction(dispatch, id),
[dispatch, id],
);
const unsetFocusedFilter = useCallback(() => dispatchFocusAction(dispatch), [
dispatch,
]);

const hooks = useMemo(
() => ({ setDataMask, setFocusedFilter, unsetFocusedFilter }),
[setDataMask, setFocusedFilter, unsetFocusedFilter],
);

const isMissingRequiredValue = checkIsMissingRequiredValue(
filter,
filter.dataMask?.filterState,
);

const setFocusedFilter = () => dispatchFocusAction(dispatch, id);
const unsetFocusedFilter = () => dispatchFocusAction(dispatch);
const filterState = useMemo(
() => ({
...filter.dataMask?.filterState,
validateStatus: isMissingRequiredValue && 'error',
}),
[filter.dataMask?.filterState, isMissingRequiredValue],
);

if (error) {
return (
Expand All @@ -208,14 +242,6 @@ const FilterValue: React.FC<FilterProps> = ({
/>
);
}
const isMissingRequiredValue = checkIsMissingRequiredValue(
filter,
filter.dataMask?.filterState,
);
const filterState = {
...filter.dataMask?.filterState,
validateStatus: isMissingRequiredValue && 'error',
};

return (
<StyledDiv data-test="form-item-value">
Expand All @@ -227,18 +253,17 @@ const FilterValue: React.FC<FilterProps> = ({
width="100%"
formData={formData}
// For charts that don't have datasource we need workaround for empty placeholder
queriesData={hasDataSource ? state : [{ data: [{}] }]}
queriesData={hasDataSource ? state : queriesDataPlaceholder}
chartType={filterType}
behaviors={[Behavior.NATIVE_FILTER]}
behaviors={behaviors}
filterState={filterState}
ownState={filter.dataMask?.ownState}
enableNoResults={metadata?.enableNoResults}
isRefreshing={isRefreshing}
hooks={{ setDataMask, setFocusedFilter, unsetFocusedFilter }}
hooks={hooks}
/>
)}
</StyledDiv>
);
};

export default FilterValue;
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import { useMemo } from 'react';
import { useSelector } from 'react-redux';
import { NativeFiltersState } from 'src/dashboard/reducers/types';
import { DataMaskStateWithId } from 'src/dataMask/types';
Expand All @@ -31,14 +32,16 @@ export function useCascadingFilters(
state => state.nativeFilters,
);
const filter = filters[id];
const cascadeParentIds: string[] = filter?.cascadeParentIds ?? [];
let cascadedFilters = {};
cascadeParentIds.forEach(parentId => {
const parentState = dataMaskSelected?.[parentId];
cascadedFilters = mergeExtraFormData(
cascadedFilters,
parentState?.extraFormData,
);
});
return cascadedFilters;
return useMemo(() => {
const cascadeParentIds: string[] = filter?.cascadeParentIds ?? [];
let cascadedFilters = {};
cascadeParentIds.forEach(parentId => {
const parentState = dataMaskSelected?.[parentId];
cascadedFilters = mergeExtraFormData(
cascadedFilters,
parentState?.extraFormData,
);
});
return cascadedFilters;
}, [dataMaskSelected, filter?.cascadeParentIds]);
}
Loading