[GitHub] [superset] geido commented on a change in pull request #17410: feat(dashboard): Add divider component in native filters

[GitBox <gi...@apache.org>]

geido commented on a change in pull request #17410:
URL: https://github.com/apache/superset/pull/17410#discussion_r749341818



##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigPane.test.tsx
##########
@@ -93,22 +94,41 @@ test('remove filter', async () => {
       }),
     );
   });
-  expect(defaultProps.onEdit).toHaveBeenCalledWith('NATIVE_FILTER-2', 'remove');
+  expect(defaultProps.onRemove).toHaveBeenCalledWith('NATIVE_FILTER-2');
 });
 
 test('add filter', async () => {
   defaultRender();
   // First trash icon
-  const removeFilterIcon = screen.getByText('Add filter')!;
+  const addButton = screen.getByText('Add')!;
+  fireEvent.mouseOver(addButton);
+  const addFilterButton = await screen.findByText('Filter');
+
   await act(async () => {
     fireEvent(
-      removeFilterIcon,
+      addFilterButton,
       new MouseEvent('click', {
         bubbles: true,
         cancelable: true,
       }),
     );
   });
+  expect(defaultProps.onAdd).toHaveBeenCalledWith('NATIVE_FILTER');
+});
 
-  expect(defaultProps.onEdit).toHaveBeenCalledWith('', 'add');
+test('add divider', async () => {
+  defaultRender();
+  const addButton = screen.getByText('Add')!;
+  fireEvent.mouseOver(addButton);
+  const addFilterButton = await screen.findByText('Divider');
+  await act(async () => {
+    fireEvent(
+      addFilterButton,
+      new MouseEvent('click', {
+        bubbles: true,
+        cancelable: true,
+      }),
+    );
+  });
+  expect(defaultProps.onAdd).toHaveBeenCalledWith('DIVIDER');
 });

Review comment:
       Is it possible to use the `userEvent` methods instead of `fireEvent`?

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx
##########
@@ -320,7 +323,14 @@ const FilterBar: React.FC<FiltersBarProps> = ({
             activeKey={editFilterSetId ? TabIds.AllFilters : undefined}
           >
             <Tabs.TabPane
-              tab={t(`All Filters (${filterValues.length})`)}
+              tab={t(
+                `All Filters (${
+                  filterValues.filter(
+                    filterValue =>
+                      filterValue.type === NativeFilterType.NATIVE_FILTER,
+                  ).length
+                })`,
+              )}

Review comment:
       Can we move the filtering part in a const outside for the sake of readability? Also, the naming convention we adopted is `All filters`, it would be great to fix it here. Finally, I think the length itself should be outside of the localization function as it is a non-translatable value.




-- 
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