kgabryje commented on code in PR #36062:
URL: https://github.com/apache/superset/pull/36062#discussion_r2543258785
##########
superset-frontend/src/dashboard/components/DashboardBuilder/state.ts:
##########
@@ -46,12 +50,21 @@ export const useNativeFilters = () => {
const filters = useFilters();
const filterValues = useMemo(() => Object.values(filters), [filters]);
const expandFilters = getUrlParam(URL_PARAMS.expandFilters);
+ const chartCustomizations = useSelector<RootState, ChartCustomization[]>(
+ state => state.dashboardInfo.metadata?.chart_customization_config || [],
Review Comment:
Let's use EMPTY_ARRAY or shallowEqual here
##########
superset-frontend/src/dashboard/components/GroupByBadge/index.tsx:
##########
@@ -18,12 +18,11 @@
*/
import { memo, useMemo, useState, useRef } from 'react';
import { useSelector } from 'react-redux';
-import { createSelector } from '@reduxjs/toolkit';
-import { t } from '@superset-ui/core';
+import { createSelector } from 'reselect';
Review Comment:
why change this import?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -155,18 +157,24 @@ const FilterBar: FC<FiltersBarProps> = ({
hidden = false,
}) => {
const history = useHistory();
- const dataMaskApplied: DataMaskStateWithId = useNativeFiltersDataMask();
+ const dataMaskApplied: DataMaskStateWithId = useAllAppliedDataMask();
+
const [dataMaskSelected, setDataMaskSelected] =
useImmer<DataMaskStateWithId>(dataMaskApplied);
- const chartCustomizationItems = useSelector<RootState, any[]>(
+ const [pendingCustomizationDataMasks, setPendingCustomizationDataMasks] =
+ useState<Record<string, DataMask>>({});
+ const chartCustomizationValues = useSelector<RootState,
ChartCustomization[]>(
state => state.dashboardInfo.metadata?.chart_customization_config || [],
Review Comment:
same here
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/state.ts:
##########
@@ -27,19 +27,12 @@ export const useRemoveCurrentFilter = (
orderedFilters: string[],
setCurrentFilterId: Function,
) => {
- useEffect(() => {
- // if the currently viewed filter is fully removed, change to another tab
- const currentFilterRemoved = removedFilters[currentFilterId];
- if (currentFilterRemoved && !currentFilterRemoved.isPending) {
- const nextFilterId = orderedFilters
- .flat()
- .find(
- filterId => !removedFilters[filterId] && filterId !==
currentFilterId,
- );
-
- if (nextFilterId) setCurrentFilterId(nextFilterId);
- }
- }, [currentFilterId, removedFilters, orderedFilters, setCurrentFilterId]);
+ useEffect(() => {}, [
Review Comment:
empty useEffect?
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx:
##########
@@ -54,28 +54,29 @@ const DefaultValue: FC<DefaultValueProps> = ({
}) => {
const formFilter = form.getFieldValue('filters')?.[filterId];
const queriesData = formFilter?.defaultValueQueriesData;
+
+ const chartType = formFilter?.filterType;
+
+ const isTimeFilter = formFilter?.filterType === 'filter_time';
+
+ const hasQueriesData = queriesData && queriesData.length > 0;
+
const loading = hasDataset && queriesData === null;
const value = formFilter?.defaultDataMask?.filterState?.value;
const isMissingRequiredValue =
hasDefaultValue && (value === null || value === undefined);
+
return loading ? (
<Loading position="inline-centered" />
) : (
<SuperChart
height={INPUT_HEIGHT}
- width={
- formFilter?.filterType === 'filter_time'
- ? TIME_FILTER_INPUT_WIDTH
- : INPUT_WIDTH
- }
+ width={isTimeFilter ? TIME_FILTER_INPUT_WIDTH : INPUT_WIDTH}
appSection={AppSection.FilterConfigModal}
behaviors={[Behavior.NativeFilter]}
formData={formData}
- // For charts that don't have datasource we need workaround for empty
placeholder
- queriesData={
- hasDataset ? formFilter?.defaultValueQueriesData : [{ data: [{}] }]
- }
- chartType={formFilter?.filterType}
+ queriesData={hasQueriesData ? queriesData : [{ data: [{}] }]}
Review Comment:
can we memoize that default?
##########
superset-frontend/src/dashboard/components/nativeFilters/state.ts:
##########
@@ -18,34 +18,61 @@
*/
import { useSelector } from 'react-redux';
import { useCallback, useMemo } from 'react';
-import { createSelector } from '@reduxjs/toolkit';
-import { Filter, Divider, isFilterDivider } from '@superset-ui/core';
+import {
+ Filter,
+ FilterConfiguration,
+ Divider,
+ isFilterDivider,
+ NativeFilterType,
+ ChartCustomization,
+ ChartCustomizationDivider,
+ ChartCustomizationConfiguration,
+} from '@superset-ui/core';
import { ActiveTabs, DashboardLayout, RootState } from '../../types';
import { CHART_TYPE, TAB_TYPE } from '../../util/componentTypes';
-const defaultFilterConfiguration: Filter[] = [];
+export function useFilterConfiguration() {
+ return useSelector<RootState, FilterConfiguration>(state => {
+ const filters = state.nativeFilters?.filters || {};
+ return Object.values(filters).filter(
+ (
+ item: Filter | Divider | ChartCustomization |
ChartCustomizationDivider,
+ ): item is Filter | Divider =>
+ item.type === NativeFilterType.NativeFilter ||
+ item.type === NativeFilterType.Divider,
+ );
+ });
+}
-const selectFilterConfiguration = createSelector(
- (state: RootState) =>
- state.dashboardInfo?.metadata?.native_filter_configuration,
- (nativeFilterConfig): (Filter | Divider)[] => {
- if (!nativeFilterConfig) {
- return defaultFilterConfiguration;
- }
- return nativeFilterConfig.filter(
- (filter: any) => filter.type !== 'CHART_CUSTOMIZATION',
+export function useChartCustomizationConfiguration() {
+ const allCustomizations = useSelector<any, ChartCustomizationConfiguration>(
+ state => state.dashboardInfo.metadata?.chart_customization_config || [],
Review Comment:
EMPTY_ARRAY
##########
superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.ts:
##########
@@ -50,7 +49,9 @@ const useFilterFocusHighlightStyles = (chartId: number) => {
const nativeFilters = useSelector((state: RootState) => state.nativeFilters);
const slices =
useSelector((state: RootState) => state.sliceEntities.slices) || {};
- const chartCustomizationItems = useSelector(selectChartCustomizationItems);
+ const chartCustomizationItems = useSelector<RootState, ChartCustomization[]>(
+ state => state.dashboardInfo.metadata?.chart_customization_config || [],
Review Comment:
EMPTY_ARRAY
##########
superset-frontend/src/dashboard/util/calculateScopes.ts:
##########
@@ -0,0 +1,80 @@
+/**
+ * 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 { NativeFilterScope } from '@superset-ui/core';
+import { LayoutItem } from '../types';
+import { CHART_TYPE } from './componentTypes';
+import { getChartIdsInFilterScope } from './getChartIdsInFilterScope';
+import { findTabsWithChartsInScope } from '../components/nativeFilters/utils';
+
+export interface ScopeItem {
+ id: string;
+ scope?: NativeFilterScope;
+}
+
+export interface CalculatedScope {
+ id: string;
+ chartsInScope: number[];
+ tabsInScope: string[];
+}
+
+export function calculateScopes<T extends ScopeItem>(
Review Comment:
dashboardLayout can be quite a big object in large dashboards. I think
`calculateScopes` should only take CHART_TYPE layout items as argument, and let
the caller do the filtering. This should help avoid perf regressions if it
turns out this function is called multiple times due to rerenders
##########
superset-frontend/src/dashboard/components/nativeFilters/state.ts:
##########
@@ -18,34 +18,61 @@
*/
import { useSelector } from 'react-redux';
import { useCallback, useMemo } from 'react';
-import { createSelector } from '@reduxjs/toolkit';
-import { Filter, Divider, isFilterDivider } from '@superset-ui/core';
+import {
+ Filter,
+ FilterConfiguration,
+ Divider,
+ isFilterDivider,
+ NativeFilterType,
+ ChartCustomization,
+ ChartCustomizationDivider,
+ ChartCustomizationConfiguration,
+} from '@superset-ui/core';
import { ActiveTabs, DashboardLayout, RootState } from '../../types';
import { CHART_TYPE, TAB_TYPE } from '../../util/componentTypes';
-const defaultFilterConfiguration: Filter[] = [];
+export function useFilterConfiguration() {
+ return useSelector<RootState, FilterConfiguration>(state => {
+ const filters = state.nativeFilters?.filters || {};
+ return Object.values(filters).filter(
+ (
+ item: Filter | Divider | ChartCustomization |
ChartCustomizationDivider,
+ ): item is Filter | Divider =>
+ item.type === NativeFilterType.NativeFilter ||
+ item.type === NativeFilterType.Divider,
+ );
+ });
+}
-const selectFilterConfiguration = createSelector(
- (state: RootState) =>
- state.dashboardInfo?.metadata?.native_filter_configuration,
- (nativeFilterConfig): (Filter | Divider)[] => {
- if (!nativeFilterConfig) {
- return defaultFilterConfiguration;
- }
- return nativeFilterConfig.filter(
- (filter: any) => filter.type !== 'CHART_CUSTOMIZATION',
+export function useChartCustomizationConfiguration() {
+ const allCustomizations = useSelector<any, ChartCustomizationConfiguration>(
+ state => state.dashboardInfo.metadata?.chart_customization_config || [],
+ );
+
+ const dashboardLayout = useDashboardLayout();
+
+ return useMemo(() => {
+ const dashboardChartIds = new Set(
+ Object.values(dashboardLayout)
Review Comment:
Let's pre-filter dashboardLayout as this calculation might be expensive on a
large dashboard
##########
superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.ts:
##########
@@ -50,7 +49,9 @@ const useFilterFocusHighlightStyles = (chartId: number) => {
const nativeFilters = useSelector((state: RootState) => state.nativeFilters);
const slices =
useSelector((state: RootState) => state.sliceEntities.slices) || {};
- const chartCustomizationItems = useSelector(selectChartCustomizationItems);
+ const chartCustomizationItems = useSelector<RootState, ChartCustomization[]>(
+ state => state.dashboardInfo.metadata?.chart_customization_config || [],
Review Comment:
and maybe EMPTY_OBJECT in the selector above, though it's unlikely that
slices are undefined
##########
superset-frontend/src/dashboard/components/GroupByBadge/index.tsx:
##########
@@ -155,10 +154,7 @@ export const GroupByBadge = ({ chartId }:
GroupByBadgeProps) => {
const triggerRef = useRef<HTMLDivElement>(null);
const theme = useTheme();
- const chartCustomizationItems = useSelector<
- RootState,
- ChartCustomizationItem[]
- >(
+ const chartCustomizationItems = useSelector<RootState, ChartCustomization[]>(
({ dashboardInfo }) =>
dashboardInfo.metadata?.chart_customization_config || [],
Review Comment:
same here, lets' memoize this []
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:
##########
@@ -722,21 +1330,136 @@ function FiltersConfigModal({
form={form}
onValuesChange={handleValuesChange}
layout="vertical"
+ preserve
>
- <FilterConfigurePane
- erroredFilters={erroredFilters}
- onRemove={handleRemoveItem}
- onAdd={addFilter}
- onChange={handleTabChange}
- getFilterTitle={getFilterTitle}
- currentFilterId={currentFilterId}
- removedFilters={removedFilters}
- restoreFilter={restoreFilter}
- onRearrange={handleRearrange}
- filters={orderedFilters}
+ <div
+ css={css`
+ height: 100%;
+ display: flex;
+ `}
>
- {formList}
- </FilterConfigurePane>
+ <div
+ css={css`
+ min-width: 290px;
+ max-width: 290px;
+ border-right: 1px solid ${theme.colorSplit};
+ display: flex;
+ flex-direction: column;
+ `}
+ >
+ <div
+ css={css`
+ display: flex;
+ align-items: center;
+ padding: ${theme.sizeUnit * 3}px;
+ `}
+ >
+ <NewItemDropdown
+ onAddFilter={addFilter}
+ onAddCustomization={addChartCustomization}
+ />
+ </div>
+ <Collapse
+ activeKey={activeCollapseKeys}
+ onChange={keys => setActiveCollapseKeys(keys as string[])}
+ ghost
+ css={css`
+ flex: 1;
+ overflow: hidden;
+ .ant-collapse-content-box {
+ padding: 0;
+ }
+ `}
+ >
+ <Collapse.Panel
+ key="filters"
+ header={
+ <div>
+ {t('Filters')} ({filterIds.length})
+ </div>
+ }
+ >
+ <ItemTitlePane
+ currentItemId={
+ isFilterId(currentItemId) ? currentItemId : ''
+ }
+ items={orderedFilters}
+ removedItems={removedFilters}
+ erroredItems={erroredFilters}
+ getItemTitle={getItemTitle}
+ onChange={setActiveItem}
+ onRearrange={(dragIndex, targetIndex, id) => {
+ handleRearrangeItems(dragIndex, targetIndex, id);
+ }}
+ onRemove={handleRemoveItem}
+ restoreItem={restoreItem}
+ dataTestId="filter-title-container"
+ deleteAltText="RemoveFilter"
+ dragType={FILTER_TYPE}
+ />
+ </Collapse.Panel>
+
+ <Collapse.Panel
+ key="chartCustomizations"
+ header={
+ <div>
+ {t('Chart customizations')} (
+ {chartCustomizationIds.length})
+ </div>
+ }
+ >
+ <ItemTitlePane
+ currentItemId={
+ isChartCustomizationId(currentItemId)
+ ? currentItemId
+ : ''
+ }
+ items={orderedCustomizations}
+ removedItems={removedChartCustomizations}
+ erroredItems={erroredCustomizations}
+ getItemTitle={getItemTitle}
+ onChange={setActiveItem}
+ onRearrange={(dragIndex, targetIndex, id) => {
+ handleRearrangeItems(dragIndex, targetIndex, id);
+ }}
+ onRemove={handleRemoveItem}
+ restoreItem={restoreItem}
+ dataTestId="customization-title-container"
+ deleteAltText="RemoveCustomization"
+ dragType={CUSTOMIZATION_TYPE}
+ />
+ </Collapse.Panel>
+ </Collapse>
+ </div>
+
+ <div
+ css={css`
+ flex: 1;
+ overflow: hidden;
+ display: flex;
+ flex-direction: column;
+ `}
+ >
+ <div
+ style={{
+ display: isFilterActive ? 'flex' : 'none',
+ flexDirection: 'column',
+ flex: 1,
+ }}
+ >
+ {formList}
+ </div>
+ <div
+ style={{
+ display: isCustomizationActive ? 'flex' : 'none',
+ flexDirection: 'column',
+ flex: 1,
+ }}
+ >
+ {customizationFormList}
+ </div>
+ </div>
+ </div>
</BaseForm>
</StyledModalBody>
</ErrorBoundary>
Review Comment:
Can we split this file into multiple smaller ones? It was too big at the
start, and now it's twice as long 😄
##########
superset-frontend/src/dashboard/components/nativeFilters/state.ts:
##########
@@ -18,34 +18,61 @@
*/
import { useSelector } from 'react-redux';
import { useCallback, useMemo } from 'react';
-import { createSelector } from '@reduxjs/toolkit';
-import { Filter, Divider, isFilterDivider } from '@superset-ui/core';
+import {
+ Filter,
+ FilterConfiguration,
+ Divider,
+ isFilterDivider,
+ NativeFilterType,
+ ChartCustomization,
+ ChartCustomizationDivider,
+ ChartCustomizationConfiguration,
+} from '@superset-ui/core';
import { ActiveTabs, DashboardLayout, RootState } from '../../types';
import { CHART_TYPE, TAB_TYPE } from '../../util/componentTypes';
-const defaultFilterConfiguration: Filter[] = [];
+export function useFilterConfiguration() {
+ return useSelector<RootState, FilterConfiguration>(state => {
+ const filters = state.nativeFilters?.filters || {};
+ return Object.values(filters).filter(
Review Comment:
Let's use createSelector here
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx:
##########
@@ -105,11 +112,16 @@ const FilterValue: FC<FilterControlProps> = ({
validateStatus,
clearAllTrigger,
onClearAllComplete,
+ isCustomization = false,
}) => {
const { id, targets, filterType, adhoc_filters, time_range } = filter;
const metadata = getChartMetadataRegistry().get(filterType);
const dependencies = useFilterDependencies(id, dataMaskSelected);
const shouldRefresh = useShouldFilterRefresh();
+
+ const behaviors = [
Review Comment:
this should be memoized
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
