feat(native-filters): Hide filters which don't affect any visible charts #15063

kgabryje · 2021-06-09T14:20:20Z

SUMMARY

This PR changes the behaviour of hiding native filters out of scope, which was originally implemented in #14933.
Before, we considered filters to be "in scope" if they were placed in the last clicked tab. This approach presented several issues. The most significant issue was that in case of dashboards without top level tabs, there was no way to have charts unattached to any tab to be considered in scope once some row level tab had been clicked (as the scope was set only by clicking tabs).
The new proposal, implemented in this PR, is that we consider all visible charts to be "in scope". That means that in order to determine if a filter is in scope, we no longer check only the last clicked tab, but the whole tabs tree of the dashboard. For details, see attached recordings of the new native filters behaviour presented on dashboards with (the first video) and without (second video) top level tabs

Potential improvement: rename "Filters out of scope" section or add a label for filters in scope to make it clear for the user why specific filters are out of scope. If you have any ideas, I'm open for suggestions 🙂

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before: see #14933
After:
https://user-images.githubusercontent.com/15073128/121371437-d6de1a00-c93d-11eb-9776-5ba53f35ab33.mov
https://user-images.githubusercontent.com/15073128/121371703-0ab93f80-c93e-11eb-8b92-3d216a049450.mov

TESTING INSTRUCTIONS

Set DASHBOARD_NATIVE_FILTERS feature flag to True
Create dashboards with tabs (top level and/or row level)
Add some native filters, go crazy with their scopes
Verify that filters that don't have any visible charts in scope are moved to "Out of scope" panel
Edit native filters's scopes and dashboard's layout (add some new tabs or charts) and verify that the feature "picked up" the changes

ADDITIONAL INFORMATION

Has associated issue:
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

CC: @junlincc @villebro @michael-s-molina

junlincc · 2021-06-09T19:55:37Z

/testenv up DASHBOARD_NATIVE_FILTERS = True

github-actions · 2021-06-09T20:51:09Z

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

suddjian · 2021-06-10T00:06:21Z

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

+      // Filter is in scope if any of it's charts is visible.
+      // Chart is visible if it's placed in an active tab tree or if it's not attached to any tab.
+      // Chart is in an active tab tree if all of it's ancestors of type TAB are active
+      const isFilterInScope = cascadeFilters[index].chartsInScope?.some(


Suggested change

const isFilterInScope = cascadeFilters[index].chartsInScope?.some(

const isFilterInScope = filter.chartsInScope?.some(

Riiiight 🤦 Done

suddjian · 2021-06-10T16:22:42Z

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

+          const tabParents = chartLayoutItem?.parents.filter(
+            (parent: string) => dashboardLayout[parent].type === TAB_TYPE,
+          );
+          return (


I'd love to see this logic moved out of components into a library of redux selectors, such as selectActiveTabbedCharts(), selectFiltersInScope(), selectChartsInScopeByFilter() etc. That way the state logic will be in a central location and easier to manage.

It will also allow calculations to be memoized between different components. This is a triple-nested loop, which could become nasty on a dashboard with a lot of charts, tabs, and filters with significant numbers of charts in scope. And I know similar loops are running elsewhere in the app.

I split some of that logic and moved it to superset/superset-frontend/src/dashboard/components/nativeFilters/state.ts, but I'm not sure if that's exactly what you expected. Can you take another look?

kgabryje · 2021-06-10T16:30:45Z

/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=True

github-actions · 2021-06-10T17:25:52Z

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

codecov · 2021-06-11T10:15:12Z

Codecov Report

Merging #15063 (85ebda2) into master (3456dd5) will decrease coverage by 0.00%.
The diff coverage is 66.03%.

@@            Coverage Diff             @@
##           master   #15063      +/-   ##
==========================================
- Coverage   77.54%   77.53%   -0.01%     
==========================================
  Files         967      967              
  Lines       49759    49787      +28     
  Branches     6352     6358       +6     
==========================================
+ Hits        38585    38602      +17     
- Misses      10972    10983      +11     
  Partials      202      202

Flag	Coverage Δ
javascript	`72.43% <66.03%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
superset-frontend/src/dashboard/actions/hydrate.js	`1.72% <ø> (ø)`
...nd/src/dashboard/containers/DashboardComponent.jsx	`92.30% <ø> (ø)`
...-frontend/src/dashboard/reducers/dashboardState.js	`73.33% <0.00%> (ø)`
...t-frontend/src/dashboard/actions/dashboardState.js	`28.65% <33.33%> (ø)`
...nd/src/dashboard/components/nativeFilters/state.ts	`75.00% <64.10%> (-25.00%)`	⬇️
...d/src/dashboard/components/gridComponents/Tabs.jsx	`86.79% <85.71%> (-0.71%)`	⬇️
...ilters/FilterBar/FilterControls/FilterControls.tsx	`80.00% <100.00%> (+2.41%)`	⬆️

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 3456dd5...85ebda2. Read the comment docs.

villebro

LGTM

github-actions · 2021-06-11T12:10:39Z

Ephemeral environment shutdown and build artifacts deleted.

…rts (apache#15063) * feat(native-filters): Hide filters which don't affect any visible charts * Fix lint errors * Add comments * Code review fix * Fix tests * Refactor logic in FilterControls

kgabryje requested a review from villebro June 9, 2021 14:20

pull-request-size bot added the size/M label Jun 9, 2021

kgabryje requested a review from zhaoyongjie June 9, 2021 14:25

kgabryje closed this Jun 9, 2021

kgabryje reopened this Jun 9, 2021

junlincc added the dashboard:native-filters Related to the native filters of the Dashboard label Jun 9, 2021

suddjian reviewed Jun 10, 2021

View reviewed changes

kgabryje added 6 commits June 11, 2021 10:33

feat(native-filters): Hide filters which don't affect any visible charts

c9a4131

Fix lint errors

1efa248

Add comments

6f176a7

Code review fix

2b420cf

Fix tests

4d4d0ed

Refactor logic in FilterControls

85ebda2

kgabryje force-pushed the feat/filters-out-of-scope branch from c3fe54c to 85ebda2 Compare June 11, 2021 09:57

pull-request-size bot added size/L and removed size/M labels Jun 11, 2021

villebro approved these changes Jun 11, 2021

View reviewed changes

kgabryje merged commit 51f0d4f into apache:master Jun 11, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(native-filters): Hide filters which don't affect any visible charts #15063

feat(native-filters): Hide filters which don't affect any visible charts #15063

kgabryje commented Jun 9, 2021 •

edited

Loading

junlincc commented Jun 9, 2021

github-actions bot commented Jun 9, 2021

suddjian Jun 10, 2021

kgabryje Jun 11, 2021

suddjian Jun 10, 2021

kgabryje Jun 11, 2021 •

edited

Loading

kgabryje commented Jun 10, 2021

github-actions bot commented Jun 10, 2021

codecov bot commented Jun 11, 2021

villebro left a comment

github-actions bot commented Jun 11, 2021

	const isFilterInScope = cascadeFilters[index].chartsInScope?.some(
	const isFilterInScope = filter.chartsInScope?.some(

feat(native-filters): Hide filters which don't affect any visible charts #15063

feat(native-filters): Hide filters which don't affect any visible charts #15063

Conversation

kgabryje commented Jun 9, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

junlincc commented Jun 9, 2021

github-actions bot commented Jun 9, 2021

suddjian Jun 10, 2021

Choose a reason for hiding this comment

kgabryje Jun 11, 2021

Choose a reason for hiding this comment

suddjian Jun 10, 2021

Choose a reason for hiding this comment

kgabryje Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

kgabryje commented Jun 10, 2021

github-actions bot commented Jun 10, 2021

codecov bot commented Jun 11, 2021

Codecov Report

villebro left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 11, 2021

kgabryje commented Jun 9, 2021 •

edited

Loading

kgabryje Jun 11, 2021 •

edited

Loading