geido commented on code in PR #33769:
URL: https://github.com/apache/superset/pull/33769#discussion_r2166727261
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/ScopingTree.tsx:
##########
@@ -40,8 +48,19 @@ const buildTreeLeafTitle = (
label: string,
hasTooltip: boolean,
tooltipTitle?: string,
+ isDeckMultiChart?: boolean,
) => {
let title = <span>{label}</span>;
+
+ if (isDeckMultiChart) {
+ title = (
+ <span style={{ display: 'inline-flex', alignItems: 'center' }}>
+ <Icons.CheckSquareOutlined style={{ marginRight: 4, fontSize: 16 }} />
Review Comment:
Icons have standard sizes that you can leverage in the props. Also, let's
use the `css` prop with template literals over `style`
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.tsx:
##########
@@ -111,29 +111,49 @@ const DeckMulti = (props: DeckMultiProps) => {
(formData: QueryFormData, payload: JsonObject, viewport?: Viewport) => {
setViewport(getAdjustedViewport());
setSubSlicesLayers({});
+
payload.data.slices.forEach(
- (subslice: { slice_id: number } & JsonObject) => {
- // Filters applied to multi_deck are passed down to underlying charts
- // note that dashboard contextual information (filter_immune_slices
and such) aren't
- // taken into consideration here
- const extra_filters = [
+ (subslice: { slice_id: number } & JsonObject, payloadIndex: number) =>
{
+ const correctLayerIndex = formData.deck_slices
+ ? formData.deck_slices.indexOf(subslice.slice_id)
+ : payloadIndex;
+
+ const layerFilterScope = formData.layer_filter_scope;
+
+ const layerSpecificExtraFilters = [
...(subslice.form_data.extra_filters || []),
...(formData.extra_filters || []),
- ...(formData.extra_form_data?.filters || []),
];
- const adhoc_filters = [
+ const layerSpecificAdhocFilters = [
...(formData.adhoc_filters || []),
...(subslice.formData?.adhoc_filters || []),
- ...(formData.extra_form_data?.adhoc_filters || []),
];
+ if (layerFilterScope) {
+ const filterDataMapping = formData.filter_data_mapping || {};
+
+ Object.entries(layerFilterScope).forEach(
+ ([filterId, filterScope]: [string, any]) => {
+ if (!filterScope || filterScope.includes(correctLayerIndex)) {
Review Comment:
`ensureIsArray` util in superset ui core could help drying up this a bit
##########
superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts:
##########
@@ -125,20 +141,49 @@ export default function getFormDataWithExtraFilters({
return cachedFormData;
}
- let extraData: { extra_form_data?: JsonObject } = {};
const activeFilters = getAllActiveFilters({
chartConfiguration,
- dataMask,
nativeFilters,
+ dataMask,
allSliceIds,
});
+
+ let extraData: JsonObject = {};
const filterIdsAppliedOnChart = Object.entries(activeFilters)
.filter(([, { scope }]) => scope.includes(chart.id))
.map(([filterId]) => filterId);
+
if (filterIdsAppliedOnChart.length) {
+ const aggregatedFormData = getExtraFormData(
+ dataMask,
+ filterIdsAppliedOnChart,
+ );
extraData = {
- extra_form_data: getExtraFormData(dataMask, filterIdsAppliedOnChart),
+ extra_form_data: aggregatedFormData,
};
+
+ if (chart.form_data?.viz_type === 'deck_multi') {
Review Comment:
I saw we are using this check in a few places and I am wondering if this is
the right way. I am worried that we are tying logics that are meant to be
generic to specific plugins now. Is there a better way to apply the required
logics so that do not feel so tied to the `deck_multi` viz type only?
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/ScopingTree.tsx:
##########
@@ -18,14 +18,22 @@
*/
import { FC, useMemo, useState, memo } from 'react';
-import { NativeFilterScope } from '@superset-ui/core';
+import { NativeFilterScope, styled } from '@superset-ui/core';
import { Tree } from 'src/components';
import { DASHBOARD_ROOT_ID } from 'src/dashboard/util/constants';
import { Tooltip } from 'src/components/Tooltip';
import { Icons } from 'src/components/Icons';
+import { Layout } from 'src/dashboard/types';
import { useFilterScopeTree } from './state';
import { findFilterScope, getTreeCheckedItems } from './utils';
+const StyledTree = styled(Tree)`
+ .antd5-tree-title {
Review Comment:
When rebasing with master all class names should now use the standard `ant`
prefix. `antd5` prefix is gone as the migration has been finalized.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils.ts:
##########
@@ -37,6 +37,52 @@ export const getNodeTitle = (node: LayoutItem) =>
node?.id?.toString?.() ??
'';
+export const generateChartSubNodes = (
+ chartId: number,
+ chart: any,
Review Comment:
I think we might be able to find existing type definitions in the codebase
to re-use. Let's avoid `any` whenever possible
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
