[GitHub] [superset] kgabryje commented on a diff in pull request #23138: feat: Cross Filters in FilterBar

["kgabryje (via GitHub)" <gi...@apache.org>]

kgabryje commented on code in PR #23138:
URL: https://github.com/apache/superset/pull/23138#discussion_r1114130386


##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarCrossFilters/CrossFilter.tsx:
##########
@@ -0,0 +1,229 @@
+/**

Review Comment:
   That's a really minor nitpick but... since we're already in `FilterBar` directory, maybe we should name the subdirectory `CrossFilters` instead of `FilterBarCrossFilters`?



##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarCrossFilters/CrossFilter.tsx:
##########
@@ -0,0 +1,229 @@
+/**
+ * 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 React, { useCallback } from 'react';
+import { styled, t, css, useTheme } from '@superset-ui/core';
+import { CrossFilterIndicator } from 'src/dashboard/components/nativeFilters/selectors';
+import Icons from 'src/components/Icons';
+import { useDispatch } from 'react-redux';
+import { setFocusedNativeFilter } from 'src/dashboard/actions/nativeFilters';
+import { Tag } from 'src/components';
+import { Tooltip } from 'src/components/Tooltip';
+import useCSSTextTruncation from 'src/hooks/useTruncation/useCSSTextTruncation';
+import { FilterBarOrientation } from 'src/dashboard/types';
+import { updateDataMask } from 'src/dataMask/actions';
+
+const StyledCrossFilterTitle = styled.div`
+  ${({ theme }) => `
+    display: flex;
+    font-size: ${theme.typography.sizes.s}px;
+    color: ${theme.colors.grayscale.base};
+    vertical-align: middle;
+    cursor: pointer;
+    align-items: center;
+  `}
+`;
+const StyledIconSearch = styled(Icons.SearchOutlined)`
+  ${({ theme }) => `
+    color: ${theme.colors.grayscale.light1};
+    margin-left: ${theme.gridUnit * 2}px;
+    &:hover {
+      color: ${theme.colors.grayscale.base};
+    }
+  `}
+`;
+
+const ellipsisCss = css`
+  white-space: nowrap;
+  overflow: hidden;
+  text-overflow: ellipsis;
+  display: inline-block;
+  vertical-align: middle;
+`;
+
+const StyledCrossFilterValue = styled('b')`
+  ${({ theme }) => `
+    max-width: ${theme.gridUnit * 25}px;
+  `}
+  ${ellipsisCss}
+`;
+
+const StyledCrossFilterColumn = styled('span')`
+  ${({ theme }) => `
+    max-width: ${theme.gridUnit * 25}px;
+    padding-right: ${theme.gridUnit}px;
+  `}
+  ${ellipsisCss}
+`;
+
+const CrossFilterTag = (props: {
+  filter: CrossFilterIndicator;
+  orientation: FilterBarOrientation;
+  removeCrossFilter: (filterId: number) => void;
+}) => {
+  const { filter, orientation, removeCrossFilter } = props;
+  const theme = useTheme();
+  const [columnRef, columnIsTruncated] =
+    useCSSTextTruncation<HTMLSpanElement>();
+  const [valueRef, valueIsTruncated] = useCSSTextTruncation<HTMLSpanElement>();
+
+  return (
+    <Tag
+      css={css`
+        ${orientation === FilterBarOrientation.VERTICAL
+          ? `
+            margin-top: ${theme.gridUnit * 2}px;
+          `
+          : `
+            margin-left: ${theme.gridUnit * 2}px;
+          `}
+      `}
+      closable
+      onClose={() => removeCrossFilter(filter.emitterId)}
+    >
+      <Tooltip title={columnIsTruncated ? filter.column : null}>
+        <StyledCrossFilterColumn ref={columnRef}>
+          {filter.column}
+        </StyledCrossFilterColumn>
+      </Tooltip>
+      <Tooltip title={valueIsTruncated ? filter.value : null}>
+        <StyledCrossFilterValue ref={valueRef}>
+          {filter.value}
+        </StyledCrossFilterValue>
+      </Tooltip>
+    </Tag>
+  );
+};
+
+const CrossFilterChartTitle = (props: {
+  title: string;
+  orientation: FilterBarOrientation;
+}) => {
+  const { title, orientation } = props;
+  const [titleRef, titleIsTruncated] = useCSSTextTruncation<HTMLSpanElement>();
+  const theme = useTheme();
+  return (
+    <Tooltip title={titleIsTruncated ? title : null}>
+      <span
+        css={css`
+          max-width: ${orientation === FilterBarOrientation.VERTICAL
+            ? `${theme.gridUnit * 60}px`
+            : `${theme.gridUnit * 15}px`};
+          line-height: 20px;

Review Comment:
   should we use a relative line height? Like 1.4 or something



##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx:
##########
@@ -126,37 +170,53 @@ const FilterControls: FC<FilterControlsProps> = ({
     </>
   );
 
-  const items = useMemo(
-    () =>
-      filtersInScope.map((filter, index) => ({
-        id: filter.id,
-        element: (
-          <div
-            className="filter-item-wrapper"
-            css={css`
-              flex-shrink: 0;
-            `}
-          >
-            {renderer(filter, index)}
-          </div>
-        ),
-      })),
-    [filtersInScope, renderer],
-  );
+  const items = useMemo(() => {
+    const crossFilters = selectedCrossFilters.map(c => ({
+      // a combination of filter name and chart id to account
+      // for multiple cross filters from the same chart in the future
+      id: `${c.name}${c.emitterId}`,
+      element: rendererCrossFilter(
+        c,
+        FilterBarOrientation.HORIZONTAL,
+        selectedCrossFilters.at(-1),
+      ),
+    }));
+    const nativeFiltersInScope = filtersInScope.map((filter, index) => ({
+      id: filter.id,
+      element: (
+        <div
+          className="filter-item-wrapper"
+          css={css`
+            flex-shrink: 0;
+          `}
+        >
+          {renderer(filter, index)}
+        </div>
+      ),
+    }));
+    return [...crossFilters, ...nativeFiltersInScope];
+  }, [filtersInScope, renderer, rendererCrossFilter, selectedCrossFilters]);
 
   const overflowedFiltersInScope = useMemo(
     () => filtersInScope.filter(({ id }) => overflowedIds?.includes(id)),
     [filtersInScope, overflowedIds],
   );
 
-  const activeOverflowedFiltersInScope = useMemo(
+  const overflowedCrossFilters = useMemo(
     () =>
-      overflowedFiltersInScope.filter(filter =>
-        isNativeFilterWithDataMask(filter),
+      selectedCrossFilters.filter(({ emitterId, name }) =>
+        overflowedIds?.includes(`${name}${emitterId}`),
       ),
-    [overflowedFiltersInScope],
+    [overflowedIds, selectedCrossFilters],
   );
 
+  const activeOverflowedFiltersInScope = useMemo(() => {
+    const activerOverflowedFilters = overflowedFiltersInScope.filter(filter =>

Review Comment:
   typo 🙂 `activerOverflowedFilters` -> `activeOverflowedFilters`



##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarCrossFilters/CrossFilter.tsx:
##########
@@ -0,0 +1,229 @@
+/**
+ * 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 React, { useCallback } from 'react';
+import { styled, t, css, useTheme } from '@superset-ui/core';
+import { CrossFilterIndicator } from 'src/dashboard/components/nativeFilters/selectors';
+import Icons from 'src/components/Icons';
+import { useDispatch } from 'react-redux';
+import { setFocusedNativeFilter } from 'src/dashboard/actions/nativeFilters';
+import { Tag } from 'src/components';
+import { Tooltip } from 'src/components/Tooltip';
+import useCSSTextTruncation from 'src/hooks/useTruncation/useCSSTextTruncation';
+import { FilterBarOrientation } from 'src/dashboard/types';
+import { updateDataMask } from 'src/dataMask/actions';
+
+const StyledCrossFilterTitle = styled.div`
+  ${({ theme }) => `
+    display: flex;
+    font-size: ${theme.typography.sizes.s}px;
+    color: ${theme.colors.grayscale.base};
+    vertical-align: middle;
+    cursor: pointer;
+    align-items: center;
+  `}
+`;
+const StyledIconSearch = styled(Icons.SearchOutlined)`

Review Comment:
   Maybe we could add a transition to make changing the colour smoother?



##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarCrossFilters/CrossFilter.tsx:
##########
@@ -0,0 +1,229 @@
+/**
+ * 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 React, { useCallback } from 'react';
+import { styled, t, css, useTheme } from '@superset-ui/core';
+import { CrossFilterIndicator } from 'src/dashboard/components/nativeFilters/selectors';
+import Icons from 'src/components/Icons';
+import { useDispatch } from 'react-redux';
+import { setFocusedNativeFilter } from 'src/dashboard/actions/nativeFilters';
+import { Tag } from 'src/components';
+import { Tooltip } from 'src/components/Tooltip';
+import useCSSTextTruncation from 'src/hooks/useTruncation/useCSSTextTruncation';
+import { FilterBarOrientation } from 'src/dashboard/types';
+import { updateDataMask } from 'src/dataMask/actions';
+
+const StyledCrossFilterTitle = styled.div`
+  ${({ theme }) => `
+    display: flex;
+    font-size: ${theme.typography.sizes.s}px;
+    color: ${theme.colors.grayscale.base};
+    vertical-align: middle;
+    cursor: pointer;
+    align-items: center;
+  `}
+`;
+const StyledIconSearch = styled(Icons.SearchOutlined)`
+  ${({ theme }) => `
+    color: ${theme.colors.grayscale.light1};
+    margin-left: ${theme.gridUnit * 2}px;
+    &:hover {
+      color: ${theme.colors.grayscale.base};
+    }
+  `}
+`;
+
+const ellipsisCss = css`
+  white-space: nowrap;
+  overflow: hidden;
+  text-overflow: ellipsis;
+  display: inline-block;
+  vertical-align: middle;
+`;
+
+const StyledCrossFilterValue = styled('b')`

Review Comment:
   
   ```suggestion
   const StyledCrossFilterValue = styled.b`
   ```



##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarCrossFilters/Vertical.tsx:
##########
@@ -0,0 +1,104 @@
+/**
+ * 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 React, { useMemo } from 'react';
+import Collapse from 'src/components/Collapse';
+import { styled, t, DataMaskStateWithId } from '@superset-ui/core';
+import { useSelector } from 'react-redux';
+import {
+  DashboardInfo,
+  DashboardLayout,
+  FilterBarOrientation,
+  RootState,
+} from 'src/dashboard/types';
+import CrossFilter from './CrossFilter';
+import crossFiltersSelector from './selectors';
+
+const StyledCollapse = styled(Collapse)`
+  ${({ theme }) => `
+    .ant-collapse-item > .ant-collapse-header {
+      padding-bottom: 0;
+    }
+    .ant-collapse-item > .ant-collapse-header > .ant-collapse-arrow {
+      font-size: ${theme.typography.sizes.xs}px;
+      padding-top: ${theme.gridUnit * 3.5}px;

Review Comment:
   Could we round it to 3 or 4? I think we agreed some time ago not to use fractions of grid unit



##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarCrossFilters/CrossFilter.tsx:
##########
@@ -0,0 +1,229 @@
+/**
+ * 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 React, { useCallback } from 'react';
+import { styled, t, css, useTheme } from '@superset-ui/core';
+import { CrossFilterIndicator } from 'src/dashboard/components/nativeFilters/selectors';
+import Icons from 'src/components/Icons';
+import { useDispatch } from 'react-redux';
+import { setFocusedNativeFilter } from 'src/dashboard/actions/nativeFilters';
+import { Tag } from 'src/components';
+import { Tooltip } from 'src/components/Tooltip';
+import useCSSTextTruncation from 'src/hooks/useTruncation/useCSSTextTruncation';
+import { FilterBarOrientation } from 'src/dashboard/types';
+import { updateDataMask } from 'src/dataMask/actions';
+
+const StyledCrossFilterTitle = styled.div`
+  ${({ theme }) => `
+    display: flex;
+    font-size: ${theme.typography.sizes.s}px;
+    color: ${theme.colors.grayscale.base};
+    vertical-align: middle;
+    cursor: pointer;
+    align-items: center;
+  `}
+`;
+const StyledIconSearch = styled(Icons.SearchOutlined)`
+  ${({ theme }) => `
+    color: ${theme.colors.grayscale.light1};
+    margin-left: ${theme.gridUnit * 2}px;
+    &:hover {
+      color: ${theme.colors.grayscale.base};
+    }
+  `}
+`;
+
+const ellipsisCss = css`
+  white-space: nowrap;
+  overflow: hidden;
+  text-overflow: ellipsis;
+  display: inline-block;
+  vertical-align: middle;
+`;
+
+const StyledCrossFilterValue = styled('b')`
+  ${({ theme }) => `
+    max-width: ${theme.gridUnit * 25}px;
+  `}
+  ${ellipsisCss}
+`;
+
+const StyledCrossFilterColumn = styled('span')`
+  ${({ theme }) => `
+    max-width: ${theme.gridUnit * 25}px;
+    padding-right: ${theme.gridUnit}px;
+  `}
+  ${ellipsisCss}
+`;
+
+const CrossFilterTag = (props: {
+  filter: CrossFilterIndicator;
+  orientation: FilterBarOrientation;
+  removeCrossFilter: (filterId: number) => void;
+}) => {
+  const { filter, orientation, removeCrossFilter } = props;
+  const theme = useTheme();
+  const [columnRef, columnIsTruncated] =
+    useCSSTextTruncation<HTMLSpanElement>();
+  const [valueRef, valueIsTruncated] = useCSSTextTruncation<HTMLSpanElement>();
+
+  return (
+    <Tag
+      css={css`
+        ${orientation === FilterBarOrientation.VERTICAL
+          ? `
+            margin-top: ${theme.gridUnit * 2}px;
+          `
+          : `
+            margin-left: ${theme.gridUnit * 2}px;
+          `}
+      `}
+      closable
+      onClose={() => removeCrossFilter(filter.emitterId)}
+    >
+      <Tooltip title={columnIsTruncated ? filter.column : null}>
+        <StyledCrossFilterColumn ref={columnRef}>
+          {filter.column}
+        </StyledCrossFilterColumn>
+      </Tooltip>
+      <Tooltip title={valueIsTruncated ? filter.value : null}>
+        <StyledCrossFilterValue ref={valueRef}>
+          {filter.value}
+        </StyledCrossFilterValue>
+      </Tooltip>
+    </Tag>
+  );
+};
+
+const CrossFilterChartTitle = (props: {
+  title: string;
+  orientation: FilterBarOrientation;
+}) => {
+  const { title, orientation } = props;
+  const [titleRef, titleIsTruncated] = useCSSTextTruncation<HTMLSpanElement>();
+  const theme = useTheme();
+  return (
+    <Tooltip title={titleIsTruncated ? title : null}>
+      <span
+        css={css`
+          max-width: ${orientation === FilterBarOrientation.VERTICAL
+            ? `${theme.gridUnit * 60}px`
+            : `${theme.gridUnit * 15}px`};
+          line-height: 20px;
+          ${ellipsisCss}
+        `}
+        ref={titleRef}
+      >
+        {title}
+      </span>
+    </Tooltip>
+  );
+};
+
+const CrossFilter = (props: {
+  filter: CrossFilterIndicator;
+  orientation: FilterBarOrientation;
+  last?: boolean;
+}) => {
+  const { filter, orientation, last } = props;
+  const theme = useTheme();
+  const dispatch = useDispatch();
+
+  const onHighlightFilterSource = useCallback(
+    (path?: string[]) => {
+      if (path) {
+        dispatch(setFocusedNativeFilter(path[0]));
+      }
+    },
+    [dispatch],
+  );
+
+  const handleRemoveCrossFilter = (chartId: number) => {
+    dispatch(
+      updateDataMask(chartId, {
+        extraFormData: {
+          filters: [],
+        },
+        filterState: {
+          value: null,
+          selectedValues: null,
+        },
+      }),
+    );
+  };
+
+  return (
+    <div
+      key={`${filter.name}${filter.emitterId}`}
+      css={css`
+        ${orientation === FilterBarOrientation.VERTICAL
+          ? `
+            display: block;
+            margin-top: ${theme.gridUnit * 4}px;
+          `
+          : `
+            display: flex;
+          `}
+      `}
+    >
+      <StyledCrossFilterTitle
+        onClick={() => onHighlightFilterSource(filter.path)}
+        role="button"
+        tabIndex={0}
+      >
+        <CrossFilterChartTitle
+          title={filter.name}
+          orientation={orientation || FilterBarOrientation.HORIZONTAL}
+        />
+        <Tooltip title={t('Locate the chart')}>
+          <StyledIconSearch iconSize="s" />
+        </Tooltip>
+      </StyledCrossFilterTitle>
+      {(filter.column || filter.value) && (
+        <CrossFilterTag
+          filter={filter}
+          orientation={orientation}
+          removeCrossFilter={handleRemoveCrossFilter}
+        />
+      )}
+      {last && (

Review Comment:
   I'm wondering if that separator line should be a part of `CrossFilter` component 🤔 Maybe we could extract it from here, render all cross filters in a container and then draw that line after the container? That would also let us get rid of that `last` logic



-- 
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-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org