feat(dashboard): Add divider component in native filters #17410

m-ajay · 2021-11-11T19:45:43Z

SUMMARY

Let users create divider component to give more context to the native filters

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Enable feature flag DASHBOARD_NATIVE_FILTERS
Go to any dashboard, and click edit (✏️) near filters.
Hover over Add button on the lower left of the pop-up and select divider.
Fill in title and description and hit save
See the divider in the Filter bar

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags: DASHBOARD_NATIVE_FILTERS
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2021-11-11T20:51:55Z

Codecov Report

Merging #17410 (c9135ec) into master (c216565) will increase coverage by 0.20%.
The diff coverage is 79.86%.

@@            Coverage Diff             @@
##           master   #17410      +/-   ##
==========================================
+ Coverage   76.78%   76.98%   +0.19%     
==========================================
  Files        1048     1045       -3     
  Lines       56516    56464      -52     
  Branches     7805     7825      +20     
==========================================
+ Hits        43396    43469      +73     
+ Misses      12866    12741     -125     
  Partials      254      254

Flag	Coverage Δ
javascript	`71.09% <79.86%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...components/DashboardBuilder/DashboardContainer.tsx	`82.69% <33.33%> (-3.03%)`	⬇️
...nd/src/dashboard/components/nativeFilters/state.ts	`70.49% <47.36%> (-4.06%)`	⬇️
...mponents/nativeFilters/FiltersConfigModal/utils.ts	`65.24% <61.90%> (+0.32%)`	⬆️
...veFilters/FiltersConfigModal/DividerConfigForm.tsx	`66.66% <66.66%> (ø)`
...ilters/FilterBar/FilterControls/FilterControls.tsx	`82.69% <85.71%> (+1.84%)`	⬆️
...tiveFilters/FiltersConfigModal/FilterTitlePane.tsx	`92.85% <87.50%> (+0.85%)`	⬆️
...eFilters/FiltersConfigModal/FiltersConfigModal.tsx	`87.11% <93.33%> (-0.61%)`	⬇️
...src/dashboard/components/FiltersBadge/selectors.ts	`79.72% <100.00%> (+0.27%)`	⬆️
...ts/nativeFilters/FilterBar/FilterControls/utils.ts	`77.77% <100.00%> (+6.34%)`	⬆️
...tiveFilters/FilterBar/FilterSets/FiltersHeader.tsx	`89.36% <100.00%> (+0.47%)`	⬆️
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c216565...c9135ec. Read the comment docs.

geido · 2021-11-15T13:36:15Z

/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true FEATURE_DASHBOARD_CROSS_FILTERS=true FEATURE_DASHBOARD_NATIVE_FILTERS_SET=true FEATURE_DASHBOARD_FILTERS_EXPERIMENTAL=true

github-actions · 2021-11-15T13:38:25Z

@geido Ephemeral environment spinning up at http://34.212.219.226:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

geido

Hey @m-ajay some comments:

Can we make the tooltip appear on the left side? Also, when you go over the two actions, there is no hover style. It would be great to add a little bit of style so it feels less plain.
I was able to save the Divider with an empty title by adding an empty char. Not as important, but white spaces are allowed in the description too.

Can we differentiate the filters from the dividers in the modal in some ways? For example, we might add a border to the dividers or separate them with additional spacing.
What do you think of using a tooltip for the description of the divider consistently with the filters? I am not sure if that was a conscious design decision but it feels like having a long description placed in the filter sidebar might occupy unjustified space

geido · 2021-11-15T13:46:04Z

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx

+              tab={t(
+                `All Filters (${
+                  filterValues.filter(
+                    filterValue =>
+                      filterValue.type === NativeFilterType.NATIVE_FILTER,
+                  ).length
+                })`,
+              )}


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.

geido · 2021-11-15T13:48:17Z

...frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigPane.test.tsx

+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');
 });


Is it possible to use the userEvent methods instead of fireEvent?

etr2460

a few comments

etr2460 · 2021-11-16T17:05:35Z

...-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx

+      if (filter.type === NativeFilterType.DIVIDER) {
+        return (
+          <div>
+            <h3>{(filter as Divider).title}</h3>


i'm very sad that the if statement above didn't guarantee that the typing is correct :( perhaps it's safer to do 'title' in filter && filter.title here instead?

etr2460 · 2021-11-16T17:06:28Z

superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitlePane.tsx

  removedFilters: Record<string, FilterRemoval>;
  currentFilterId: string;
  filterGroups: string[][];
  erroredFilters: string[];
 }

+const StyledI = styled.div`


Can we have a better name for this? not sure what an I is

etr2460 · 2021-11-16T17:07:39Z

...set-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/SectionConfigForm.tsx

+  </Container>
+);
+
+export default SectionConfigForm;


should this be called DividerConfigForm? Let's be consistent throughout

etr2460 · 2021-11-16T17:09:11Z

superset-frontend/src/dashboard/components/nativeFilters/types.ts

+  id: string;
+  title: string;
+  description: string;
+  type: NativeFilterType;


I think if you set type: typeof NativeFilterType.DIVIDER (or something like that, maybe even just type: 'DIVIDER') then you won't need to do type coercion above anymore

m-ajay · 2021-11-18T19:33:50Z

@geido Thanks for the comments.
For comment 3, do you have any mockup? I tried to add some spacing in the modal, but, due to limited space, it did not look good.
About comment 4 - I'm not sure if we need a tool tip. I feel like user should be responsible for adding the content in a way that it doesn't break the filter bar. Even if we are adding a tooltip, we will have to truncate the content in an arbitrary length. I'm not sure if we should do that.

etr2460

one more comment, otherwise lgtm!

etr2460 · 2021-11-22T18:44:57Z

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx

@@ -320,7 +323,7 @@ const FilterBar: React.FC<FiltersBarProps> = ({
            activeKey={editFilterSetId ? TabIds.AllFilters : undefined}
          >
            <Tabs.TabPane
-              tab={t(`All Filters (${filterValues.length})`)}
+              tab={`${t('All filters')} (${numberOfFilters})`}


i know you didn't add this (and instead are just touching it to update the casing of the string), but this translation string isn't correct. you need something like:

t('All filters (%(numberOfFilters))', numberOfFilters)

see here for more details: https://github.com/apache-superset/superset-ui/tree/master/packages/superset-ui-core/src/translation#api

you'll also want to change the one on line 346

github-actions · 2021-11-24T00:14:18Z

Ephemeral environment shutdown and build artifacts deleted.

* Tests are working, type errors are fixed * Fix filterset * add license header to the new file * fix test * PR comments * Linting * test fix * small fix

m-ajay added 3 commits November 9, 2021 12:18

Tests are working, type errors are fixed

3192ce5

Fix filterset

b110bab

More tests

1e3305a

pull-request-size bot added the size/XL label Nov 11, 2021

m-ajay changed the title ~~Feat/add filter description component~~ feat(dashboard): Add divider component in native filters Nov 11, 2021

m-ajay added 2 commits November 11, 2021 11:48

add license header to the new file

5030c9f

fix test

554dc92

m-ajay marked this pull request as ready for review November 11, 2021 20:08

junlincc requested review from simcha90, kgabryje, amitmiran137 and riahk and removed request for simcha90 November 11, 2021 23:15

geido reviewed Nov 15, 2021

View reviewed changes

etr2460 reviewed Nov 16, 2021

View reviewed changes

m-ajay added 3 commits November 16, 2021 10:08

Merge branch 'master' into feat/add-filter-description-component

667c07e

PR comments

e3161a2

conflict resolution

76909f6

Linting

a67255b

m-ajay requested review from etr2460 and geido November 18, 2021 21:47

m-ajay added 2 commits November 19, 2021 11:56

test fix

ef76009

Merge branch 'master' into feat/add-filter-description-component

22ce71f

etr2460 approved these changes Nov 22, 2021

View reviewed changes

conflict resolution

4f6c7b4

small fix

c9135ec

etr2460 merged commit 9576478 into apache:master Nov 24, 2021

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dashboard): Add divider component in native filters #17410

feat(dashboard): Add divider component in native filters #17410

m-ajay commented Nov 11, 2021 •

edited

Loading

codecov bot commented Nov 11, 2021 •

edited

Loading

geido commented Nov 15, 2021

github-actions bot commented Nov 15, 2021

geido left a comment

geido Nov 15, 2021

geido Nov 15, 2021

etr2460 left a comment

etr2460 Nov 16, 2021

etr2460 Nov 16, 2021

etr2460 Nov 16, 2021

etr2460 Nov 16, 2021

m-ajay commented Nov 18, 2021

etr2460 left a comment

etr2460 Nov 22, 2021

github-actions bot commented Nov 24, 2021

feat(dashboard): Add divider component in native filters #17410

feat(dashboard): Add divider component in native filters #17410

Conversation

m-ajay commented Nov 11, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Nov 11, 2021 • edited Loading

Codecov Report

geido commented Nov 15, 2021

github-actions bot commented Nov 15, 2021

geido left a comment

Choose a reason for hiding this comment

geido Nov 15, 2021

Choose a reason for hiding this comment

geido Nov 15, 2021

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

etr2460 Nov 16, 2021

Choose a reason for hiding this comment

etr2460 Nov 16, 2021

Choose a reason for hiding this comment

etr2460 Nov 16, 2021

Choose a reason for hiding this comment

etr2460 Nov 16, 2021

Choose a reason for hiding this comment

m-ajay commented Nov 18, 2021

etr2460 left a comment

Choose a reason for hiding this comment

etr2460 Nov 22, 2021

Choose a reason for hiding this comment

github-actions bot commented Nov 24, 2021

m-ajay commented Nov 11, 2021 •

edited

Loading

codecov bot commented Nov 11, 2021 •

edited

Loading