From ed135330cdcc291c4cbda4810819151c4ebab504 Mon Sep 17 00:00:00 2001
From: Diego Medina <diegomedina24@gmail.com>
Date: Mon, 7 Mar 2022 21:48:21 -0300
Subject: [PATCH 1/2] feat: Scroll to bottom when adding a new native filter
 and the page is filled

---
 .../FiltersConfigModal/DraggableFilter.tsx    |   6 +-
 .../FilterConfigurePane.tsx                   |   2 +-
 .../FilterTitleContainer.tsx                  | 232 +++++++++---------
 .../FiltersConfigModal/FilterTitlePane.tsx    |  27 +-
 .../FiltersConfigModal/FiltersConfigModal.tsx |   4 +-
 5 files changed, 144 insertions(+), 127 deletions(-)

diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/DraggableFilter.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/DraggableFilter.tsx
index 188655355ca45..7a4827c80bcf0 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/DraggableFilter.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/DraggableFilter.tsx
@@ -57,7 +57,7 @@ const DragIcon = styled(Icons.Drag, {
 interface FilterTabTitleProps {
   index: number;
   filterIds: string[];
-  onRearrage: (dragItemIndex: number, targetIndex: number) => void;
+  onRearrange: (dragItemIndex: number, targetIndex: number) => void;
 }
 
 interface DragItem {
@@ -68,7 +68,7 @@ interface DragItem {
 
 export const DraggableFilter: React.FC<FilterTabTitleProps> = ({
   index,
-  onRearrage,
+  onRearrange,
   filterIds,
   children,
 }) => {
@@ -120,7 +120,7 @@ export const DraggableFilter: React.FC<FilterTabTitleProps> = ({
         return;
       }
 
-      onRearrage(dragIndex, hoverIndex);
+      onRearrange(dragIndex, hoverIndex);
       // Note: we're mutating the monitor item here.
       // Generally it's better to avoid mutations,
       // but it's good here for the sake of performance
diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx
index a65e167fdf39b..38328478313c6 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx
@@ -75,7 +75,7 @@ const FiltureConfigurePane: React.FC<Props> = ({
           getFilterTitle={getFilterTitle}
           onChange={onChange}
           onAdd={(type: NativeFilterType) => onAdd(type)}
-          onRearrage={onRearrange}
+          onRearrange={onRearrange}
           onRemove={(id: string) => onRemove(id)}
           restoreFilter={restoreFilter}
         />
diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitleContainer.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitleContainer.tsx
index 6ef40d8303b57..dd870a5e8c5b4 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitleContainer.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitleContainer.tsx
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import React from 'react';
+import React, { forwardRef } from 'react';
 import { styled, t } from '@superset-ui/core';
 import Icons from 'src/components/Icons';
 import { FilterRemoval } from './types';
@@ -72,124 +72,130 @@ interface Props {
   removedFilters: Record<string, FilterRemoval>;
   onRemove: (id: string) => void;
   restoreFilter: (id: string) => void;
-  onRearrage: (dragIndex: number, targetIndex: number) => void;
+  onRearrange: (dragIndex: number, targetIndex: number) => void;
   filters: string[];
   erroredFilters: string[];
 }
 
-const FilterTitleContainer: React.FC<Props> = ({
-  getFilterTitle,
-  onChange,
-  onRemove,
-  restoreFilter,
-  onRearrage,
-  currentFilterId,
-  removedFilters,
-  filters,
-  erroredFilters = [],
-}) => {
-  const renderComponent = (id: string) => {
-    const isRemoved = !!removedFilters[id];
-    const isErrored = erroredFilters.includes(id);
-    const isActive = currentFilterId === id;
-    const classNames = [];
-    if (isErrored) {
-      classNames.push('errored');
-    }
-    if (isActive) {
-      classNames.push('active');
-    }
-    return (
-      <FilterTitle
-        role="tab"
-        key={`filter-title-tab-${id}`}
-        onClick={() => onChange(id)}
-        className={classNames.join(' ')}
-      >
-        <div css={{ display: 'flex', width: '100%' }}>
-          <div css={{ alignItems: 'center', display: 'flex' }}>
-            {isRemoved ? t('(Removed)') : getFilterTitle(id)}
+const FilterTitleContainer = forwardRef<HTMLDivElement, Props>(
+  (
+    {
+      getFilterTitle,
+      onChange,
+      onRemove,
+      restoreFilter,
+      onRearrange,
+      currentFilterId,
+      removedFilters,
+      filters,
+      erroredFilters = [],
+    },
+    ref,
+  ) => {
+    const renderComponent = (id: string) => {
+      const isRemoved = !!removedFilters[id];
+      const isErrored = erroredFilters.includes(id);
+      const isActive = currentFilterId === id;
+      const classNames = [];
+      if (isErrored) {
+        classNames.push('errored');
+      }
+      if (isActive) {
+        classNames.push('active');
+      }
+      return (
+        <FilterTitle
+          role="tab"
+          key={`filter-title-tab-${id}`}
+          onClick={() => onChange(id)}
+          className={classNames.join(' ')}
+        >
+          <div css={{ display: 'flex', width: '100%' }}>
+            <div css={{ alignItems: 'center', display: 'flex' }}>
+              {isRemoved ? t('(Removed)') : getFilterTitle(id)}
+            </div>
+            {!removedFilters[id] && isErrored && (
+              <StyledWarning className="warning" />
+            )}
+            {isRemoved && (
+              <span
+                css={{ alignSelf: 'flex-end', marginLeft: 'auto' }}
+                role="button"
+                data-test="undo-button"
+                tabIndex={0}
+                onClick={e => {
+                  e.preventDefault();
+                  restoreFilter(id);
+                }}
+              >
+                {t('Undo?')}
+              </span>
+            )}
           </div>
-          {!removedFilters[id] && isErrored && (
-            <StyledWarning className="warning" />
-          )}
-          {isRemoved && (
-            <span
-              css={{ alignSelf: 'flex-end', marginLeft: 'auto' }}
-              role="button"
-              data-test="undo-button"
-              tabIndex={0}
-              onClick={e => {
-                e.preventDefault();
-                restoreFilter(id);
-              }}
-            >
-              {t('Undo?')}
-            </span>
-          )}
-        </div>
-        <div css={{ alignSelf: 'flex-start', marginLeft: 'auto' }}>
-          {isRemoved ? null : (
-            <StyledTrashIcon
-              onClick={event => {
-                event.stopPropagation();
-                onRemove(id);
-              }}
-              alt="RemoveFilter"
-            />
-          )}
-        </div>
-      </FilterTitle>
-    );
-  };
-  const recursivelyRender = (
-    elementId: string,
-    nodeList: Array<{ id: string; parentId: string | null }>,
-    rendered: Array<string>,
-  ): React.ReactNode => {
-    const didAlreadyRender = rendered.indexOf(elementId) >= 0;
-    if (didAlreadyRender) {
-      return null;
-    }
-    let parent = null;
-    const element = nodeList.filter(el => el.id === elementId)[0];
-    if (!element) {
-      return null;
-    }
-
-    rendered.push(elementId);
-    if (element.parentId) {
-      parent = recursivelyRender(element.parentId, nodeList, rendered);
-    }
-    const children = nodeList
-      .filter(item => item.parentId === elementId)
-      .map(item => recursivelyRender(item.id, nodeList, rendered));
-    return (
-      <>
-        {parent}
-        {renderComponent(elementId)}
-        {children}
-      </>
-    );
-  };
+          <div css={{ alignSelf: 'flex-start', marginLeft: 'auto' }}>
+            {isRemoved ? null : (
+              <StyledTrashIcon
+                onClick={event => {
+                  event.stopPropagation();
+                  onRemove(id);
+                }}
+                alt="RemoveFilter"
+              />
+            )}
+          </div>
+        </FilterTitle>
+      );
+    };
+    const recursivelyRender = (
+      elementId: string,
+      nodeList: Array<{ id: string; parentId: string | null }>,
+      rendered: Array<string>,
+    ): React.ReactNode => {
+      const didAlreadyRender = rendered.indexOf(elementId) >= 0;
+      if (didAlreadyRender) {
+        return null;
+      }
+      let parent = null;
+      const element = nodeList.filter(el => el.id === elementId)[0];
+      if (!element) {
+        return null;
+      }
 
-  const renderFilterGroups = () => {
-    const items: React.ReactNode[] = [];
-    filters.forEach((item, index) => {
-      items.push(
-        <DraggableFilter
-          key={index}
-          onRearrage={onRearrage}
-          index={index}
-          filterIds={[item]}
-        >
-          {renderComponent(item)}
-        </DraggableFilter>,
+      rendered.push(elementId);
+      if (element.parentId) {
+        parent = recursivelyRender(element.parentId, nodeList, rendered);
+      }
+      const children = nodeList
+        .filter(item => item.parentId === elementId)
+        .map(item => recursivelyRender(item.id, nodeList, rendered));
+      return (
+        <>
+          {parent}
+          {renderComponent(elementId)}
+          {children}
+        </>
       );
-    });
-    return items;
-  };
-  return <Container>{renderFilterGroups()}</Container>;
-};
+    };
+
+    const renderFilterGroups = () => {
+      const items: React.ReactNode[] = [];
+      filters.forEach((item, index) => {
+        items.push(
+          <DraggableFilter
+            key={index}
+            onRearrange={onRearrange}
+            index={index}
+            filterIds={[item]}
+          >
+            {renderComponent(item)}
+          </DraggableFilter>,
+        );
+      });
+      return items;
+    };
+
+    return <Container ref={ref}>{renderFilterGroups()}</Container>;
+  },
+);
 
 export default FilterTitleContainer;
diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitlePane.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitlePane.tsx
index 5681a41717666..1c9645ecde89a 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitlePane.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitlePane.tsx
@@ -16,8 +16,8 @@
  * specific language governing permissions and limitations
  * under the License.
  */
+import React, { useRef } from 'react';
 import { NativeFilterType, styled, t, useTheme } from '@superset-ui/core';
-import React from 'react';
 import { AntdDropdown } from 'src/components';
 import { MainNav as Menu } from 'src/components/Menu';
 import FilterTitleContainer from './FilterTitleContainer';
@@ -26,7 +26,7 @@ import { FilterRemoval } from './types';
 interface Props {
   restoreFilter: (id: string) => void;
   getFilterTitle: (id: string) => string;
-  onRearrage: (dragIndex: number, targetIndex: number) => void;
+  onRearrange: (dragIndex: number, targetIndex: number) => void;
   onRemove: (id: string) => void;
   onChange: (id: string) => void;
   onAdd: (type: NativeFilterType) => void;
@@ -52,23 +52,26 @@ const TabsContainer = styled.div`
   flex-direction: column;
 `;
 
+const options = [
+  { label: 'Filter', type: NativeFilterType.NATIVE_FILTER },
+  { label: 'Divider', type: NativeFilterType.DIVIDER },
+];
+
 const FilterTitlePane: React.FC<Props> = ({
   getFilterTitle,
   onChange,
   onAdd,
   onRemove,
-  onRearrage,
+  onRearrange,
   restoreFilter,
   currentFilterId,
   filters,
   removedFilters,
   erroredFilters,
 }) => {
+  const filtersContainerRef = useRef<HTMLDivElement>(null);
   const theme = useTheme();
-  const options = [
-    { label: 'Filter', type: NativeFilterType.NATIVE_FILTER },
-    { label: 'Divider', type: NativeFilterType.DIVIDER },
-  ];
+
   const handleOnAdd = (type: NativeFilterType) => {
     onAdd(type);
     setTimeout(() => {
@@ -77,6 +80,13 @@ const FilterTitlePane: React.FC<Props> = ({
         const navList = element.getElementsByClassName('ant-tabs-nav-list')[0];
         navList.scrollTop = navList.scrollHeight;
       }
+
+      if (filtersContainerRef?.current) {
+        filtersContainerRef.current.scroll({
+          top: filtersContainerRef.current.scrollHeight,
+          behavior: 'smooth',
+        });
+      }
     }, 0);
   };
   const menu = (
@@ -109,6 +119,7 @@ const FilterTitlePane: React.FC<Props> = ({
         }}
       >
         <FilterTitleContainer
+          ref={filtersContainerRef}
           filters={filters}
           currentFilterId={currentFilterId}
           removedFilters={removedFilters}
@@ -116,7 +127,7 @@ const FilterTitlePane: React.FC<Props> = ({
           erroredFilters={erroredFilters}
           onChange={onChange}
           onRemove={onRemove}
-          onRearrage={onRearrage}
+          onRearrange={onRearrange}
           restoreFilter={restoreFilter}
         />
       </div>
diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
index fd3ac06ea6db9..1c3906944ba5b 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
@@ -379,7 +379,7 @@ export function FiltersConfigModal({
       handleConfirmCancel();
     }
   };
-  const onRearrage = (dragIndex: number, targetIndex: number) => {
+  const onRearrange = (dragIndex: number, targetIndex: number) => {
     const newOrderedFilter = [...orderedFilters];
     const removed = newOrderedFilter.splice(dragIndex, 1)[0];
     newOrderedFilter.splice(targetIndex, 0, removed);
@@ -531,7 +531,7 @@ export function FiltersConfigModal({
               currentFilterId={currentFilterId}
               removedFilters={removedFilters}
               restoreFilter={restoreFilter}
-              onRearrange={onRearrage}
+              onRearrange={onRearrange}
               filters={orderedFilters}
             >
               {(id: string) => getForm(id)}

From 2b1922a0dbb778d5ebeefc890f581b46dfafe910 Mon Sep 17 00:00:00 2001
From: Diego Medina <diegomedina24@gmail.com>
Date: Wed, 9 Mar 2022 12:24:23 -0300
Subject: [PATCH 2/2] Add test

---
 .../FilterConfigPane.test.tsx                 | 47 ++++++++++++++++++-
 .../FilterConfigurePane.tsx                   |  4 +-
 .../FilterTitleContainer.tsx                  |  6 ++-
 .../FiltersConfigModal/FilterTitlePane.tsx    | 10 ++--
 .../FiltersConfigModal/FiltersConfigModal.tsx |  6 +--
 5 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigPane.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigPane.test.tsx
index 78c4d77da1918..3742d536326fb 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigPane.test.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigPane.test.tsx
@@ -22,6 +22,9 @@ import { buildNativeFilter } from 'spec/fixtures/mockNativeFilters';
 import { act, fireEvent, render, screen } from 'spec/helpers/testing-library';
 import FilterConfigPane from './FilterConfigurePane';
 
+const scrollMock = jest.fn();
+Element.prototype.scroll = scrollMock;
+
 const defaultProps = {
   children: jest.fn(),
   getFilterTitle: (id: string) => id,
@@ -56,6 +59,10 @@ function defaultRender(initialState: any = defaultState, props = defaultProps) {
   });
 }
 
+beforeEach(() => {
+  scrollMock.mockClear();
+});
+
 test('renders form', async () => {
   await act(async () => {
     defaultRender();
@@ -65,7 +72,7 @@ test('renders form', async () => {
 
 test('drag and drop', async () => {
   defaultRender();
-  // Drag the state and contry filter above the product filter
+  // Drag the state and country filter above the product filter
   const [countryStateFilter, productFilter] = document.querySelectorAll(
     'div[draggable=true]',
   );
@@ -132,3 +139,41 @@ test('add divider', async () => {
   });
   expect(defaultProps.onAdd).toHaveBeenCalledWith('DIVIDER');
 });
+
+test('filter container should scroll to bottom when adding items', async () => {
+  const state = {
+    dashboardInfo: {
+      metadata: {
+        native_filter_configuration: new Array(35)
+          .fill(0)
+          .map((_, index) =>
+            buildNativeFilter(`NATIVE_FILTER-${index}`, `filter-${index}`, []),
+          ),
+      },
+    },
+    dashboardLayout,
+  };
+  const props = {
+    ...defaultProps,
+    filters: new Array(35).fill(0).map((_, index) => `NATIVE_FILTER-${index}`),
+  };
+
+  defaultRender(state, props);
+
+  const addButton = screen.getByText('Add filters and dividers')!;
+  fireEvent.mouseOver(addButton);
+
+  const addFilterButton = await screen.findByText('Filter');
+  await act(async () => {
+    fireEvent(
+      addFilterButton,
+      new MouseEvent('click', {
+        bubbles: true,
+        cancelable: true,
+      }),
+    );
+  });
+
+  const containerElement = screen.getByTestId('filter-title-container');
+  expect(containerElement.scroll).toHaveBeenCalled();
+});
diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx
index 38328478313c6..dba7e6bb30250 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx
@@ -50,7 +50,7 @@ const TitlesContainer = styled.div`
   border-right: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
 `;
 
-const FiltureConfigurePane: React.FC<Props> = ({
+const FilterConfigurePane: React.FC<Props> = ({
   getFilterTitle,
   onChange,
   onRemove,
@@ -98,4 +98,4 @@ const FiltureConfigurePane: React.FC<Props> = ({
   );
 };
 
-export default FiltureConfigurePane;
+export default FilterConfigurePane;
diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitleContainer.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitleContainer.tsx
index dd870a5e8c5b4..f5fe459e4b260 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitleContainer.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitleContainer.tsx
@@ -194,7 +194,11 @@ const FilterTitleContainer = forwardRef<HTMLDivElement, Props>(
       return items;
     };
 
-    return <Container ref={ref}>{renderFilterGroups()}</Container>;
+    return (
+      <Container data-test="filter-title-container" ref={ref}>
+        {renderFilterGroups()}
+      </Container>
+    );
   },
 );
 
diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitlePane.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitlePane.tsx
index 1c9645ecde89a..79dc4148349aa 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitlePane.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitlePane.tsx
@@ -81,12 +81,10 @@ const FilterTitlePane: React.FC<Props> = ({
         navList.scrollTop = navList.scrollHeight;
       }
 
-      if (filtersContainerRef?.current) {
-        filtersContainerRef.current.scroll({
-          top: filtersContainerRef.current.scrollHeight,
-          behavior: 'smooth',
-        });
-      }
+      filtersContainerRef?.current?.scroll?.({
+        top: filtersContainerRef.current.scrollHeight,
+        behavior: 'smooth',
+      });
     }, 0);
   };
   const menu = (
diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
index 1c3906944ba5b..d258b34fa7489 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
@@ -38,7 +38,7 @@ import ErrorBoundary from 'src/components/ErrorBoundary';
 import { StyledModal } from 'src/components/Modal';
 import { testWithId } from 'src/utils/testUtils';
 import { useFilterConfigMap, useFilterConfiguration } from '../state';
-import FiltureConfigurePane from './FilterConfigurePane';
+import FilterConfigurePane from './FilterConfigurePane';
 import FiltersConfigForm, {
   FilterPanels,
 } from './FiltersConfigForm/FiltersConfigForm';
@@ -522,7 +522,7 @@ export function FiltersConfigModal({
             onValuesChange={onValuesChange}
             layout="vertical"
           >
-            <FiltureConfigurePane
+            <FilterConfigurePane
               erroredFilters={erroredFilters}
               onRemove={handleRemoveItem}
               onAdd={addFilter}
@@ -535,7 +535,7 @@ export function FiltersConfigModal({
               filters={orderedFilters}
             >
               {(id: string) => getForm(id)}
-            </FiltureConfigurePane>
+            </FilterConfigurePane>
           </StyledForm>
         </StyledModalBody>
       </ErrorBoundary>