feat(dashboard/native-filters): Hide filters out of scope of current tab by kgabryje · Pull Request #14933 · apache/superset

kgabryje · 2021-06-01T15:58:05Z

SUMMARY

On dashboards with tabs, native filters that are out of scope of currently focused tab should be hidden.
When user opens a dashboard with top level tabs, the active top level tab is used as an initially focused tab. For dashboards without top level tabs, initially every filter is considered to be in scope, until the user clicks on a tab.
Hidden filters are moved to a new collapsible section in the native filters panel. In order to prevent expensive rerendering of native filters when they are moved in and out of collapsible, I used react-reverse-portal library, which utilizes React portals to perform reparenting without unmounting and remounting the child. In our case, it was essential, as each native filter sends a request to backend on mount - if a user had many native filters with narrow scopes, we'd risk having lots of redundant API calls on each tab change.
Moreover, I optimized finding charts and tabs in scope for each native filter. Before, functions that traverse the whole component tree in search for charts and tabs were called every time a native filter was focused. Now, they are called once for each filter when the filters are initialized (and then again when filter's scope or dashboard layout changes).

In case of dashboards without any tabs, there are no visible changes.

Potential improvements:

Show currently focused tab's name. I'd love some design input on that, as I'm not sure where to put tab's name on the filter panel and how to make it consistent with the rest of the design. I believe that would improve user experience a lot, as it might be confusing for the user why some filters are marked as "out of scope".
Currently for dashboards without top level tabs, we start with every filter being considered to be in scope. After a user focuses on some tab, there is no way to return to the initial state. It might be problematic in cases when there are charts not attached to any tab group - after some tab has been focused, we can never return to the state where a chart outside of any tab group is "in scope".

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-06-01.at.17.50.05.mov

TESTING INSTRUCTIONS

Set DASHBOARD_NATIVE_FILTERS feature flag to True.
Create a dashboard with tabs (try both top level tabs + grid level tabs and only grid level tabs) and add some native filters with different scopes.
Verify that when you change tabs, filters that are out of scope of current tab are moved to collapsed section.
Verify that native filters are not rerendered - when you switch tabs, there shouldn't be any new api calls related to native filters.

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

junlincc · 2021-06-02T00:10:08Z

@kgabryje Thank you Kamil for implementing this(more ideal) solution.
question, out of scope filters are not disabled, correct? also if we disabled them, do you know how much improvement on performance we can get?

villebro

One minor thought on deferring loading to first display of out-of-scope filters, other than that (and failed tests) this is looks and works super well (= I wasn't able to break it)!

michael-s-molina · 2021-06-02T10:41:25Z

2- Currently for dashboards without top level tabs, we start with every filter being considered to be in scope. After a user focuses on some tab, there is no way to return to the initial state. It might be problematic in cases when there are charts not attached to any tab group - after some tab has been focused, we can never return to the state where a chart outside of any tab group is "in scope".

Shouldn’t the initial state be the filters applicable to the standalone charts and the filters applicable to the selected tab since we always select a tab by default? When we have a tab group the first tab always starts selected, so the filters should be the same as when the user manually selects it.

kgabryje · 2021-06-02T11:05:41Z

Shouldn’t the initial state be the filters applicable to the standalone charts and the filters applicable to the selected tab since we always select a tab by default? When we have a tab group the first tab always starts selected, so the filters should be the same as when the user manually selects it.

I think that our intention was to hide the filters as a reaction to user manually setting focus on some tab by clicking it. I see your point however and that makes sense to me. Let's wait for product input (CC @junlincc) and come back to that matter in the second iteration of the feature.

codecov · 2021-06-02T13:58:22Z

Codecov Report

Merging #14933 (c2bd1f5) into master (66282c3) will decrease coverage by 0.00%.
The diff coverage is 75.65%.

@@            Coverage Diff             @@
##           master   #14933      +/-   ##
==========================================
- Coverage   77.62%   77.62%   -0.01%     
==========================================
  Files         963      965       +2     
  Lines       49316    49497     +181     
  Branches     6228     6261      +33     
==========================================
+ Hits        38284    38423     +139     
- Misses      10833    10873      +40     
- Partials      199      201       +2

Flag	Coverage Δ
javascript	`72.50% <75.65%> (+0.03%)`	⬆️

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% <ø> (+7.45%)`	⬆️
...-frontend/src/dashboard/reducers/dashboardState.js	`73.33% <0.00%> (-2.53%)`	⬇️
...t-frontend/src/dashboard/actions/dashboardState.js	`28.65% <33.33%> (+0.08%)`	⬆️
...ashboard/components/gridComponents/ChartHolder.jsx	`74.71% <33.33%> (-3.07%)`	⬇️
...ilters/FilterBar/FilterControls/FilterControls.tsx	`76.78% <67.50%> (-17.96%)`	⬇️
...d/src/dashboard/components/gridComponents/Tabs.jsx	`87.37% <87.50%> (+11.06%)`	⬆️
...nd/src/dashboard/components/nativeFilters/utils.ts	`86.25% <93.54%> (+4.61%)`	⬆️
...components/DashboardBuilder/DashboardContainer.tsx	`100.00% <100.00%> (ø)`
...nd/src/dashboard/components/nativeFilters/state.ts	`100.00% <100.00%> (ø)`
... and 18 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 66282c3...c2bd1f5. Read the comment docs.

junlincc · 2021-06-02T15:47:10Z

when user add value to filters in out of scope section, does apply button get enabled?

kgabryje · 2021-06-02T16:12:59Z

when user add value to filters in out of scope section, does apply button get enabled?

Yes, we only change how the filters are displayed

villebro

LGTM! I propose merging this as-is as I feel this UX is superior to the current one in every way. We can then continue the discussions about refining this functionality later.

villebro · 2021-06-02T16:18:29Z

-  xit('should filter by published correctly', () => {
+  it('should filter by published correctly', () => {


wow, this was unexpected, bycatch fixing and re-enabling a flaky test 😄

* upstream/master: (68 commits) fix typos (apache#14950) docs: fix custom oauth config (apache#14997) fix: apply template_params on external_metadata (apache#14996) fix: toggle fullscreen on the dashboard (apache#14979) feat(native-filters): add markers and number formatter + simple tests (apache#14981) fix(native-filters): Fix "undefined" error after editing a filter (apache#14984) fix(native-filters): remove implied fetch predicate (apache#14982) fix(native-filters): update cascaded filter state on change (apache#14980) fix(filter box): replace freeform where clause with ilike (apache#14900) refactor: Convert TableElement.jsx component from class to functional with hooks (apache#14830) fix: renamed sqllab filters to _filters (apache#14971) feat(native-filters): apply cascading without instant filtering (apache#14966) chore: bump superset-ui to 0.17.53 (apache#14968) fix(native-filters): cascading filters not rendering in tab (apache#14964) feat: add type_generic and is_dttm to table metadata (apache#14863) additional safeguard (apache#14953) feat: Adding FORCE_SSL as feature flag in config.py (apache#14934) feat(dashboard/native-filters): Hide filters out of scope of current tab (apache#14933) fix: time parser truncate to first day of year/month (apache#14945) fix: is_temporal should overwrite is_dttm (apache#14894) ...

graceguo-supercat · 2021-06-12T02:07:14Z

+        tabsInScope: Array.from(tabsInScope),
+      });
+    });
+    dispatch(setFilterConfiguration(nativeFiltersValues));


@kgabryje This update function is called every time a user opens a dashboard, even without any change. This is not a correct behavior.
And, update dashboard should check user permission and get user confirm. This call is secretly changed dashboard json_metadata without user confirm. This will cause serious issue. Please fix asap. Otherwise this PR should be reverted.
cc @junlincc

graceguo-supercat · 2021-06-13T07:45:10Z

In airbnb we didn't enable dashboard native filters feature flag, but because of this PR, all the recent visited dashboard were set

"show_native_filters": true, 
"native_filter_configuration": [],

This will cause dashboard in a wrong state.

For dashboard doesn't enable native filters, it should not have above properties in the json_metadata.
When dashboard native filter feature is enabled, "native_filter_configuration": [] means dashboard didn't have any filter component.
This difference is critical for dashboard filter_box migration work. Please check SIP-64 "airbnb user" section.

I think it is necessary to create a DB migration to fix the side-effects caused by this PR. In the migration script, scan every dashboard's metadata, when dashboard native filters feature is not enabled, should remove above 2 properties from dashboard json_metadata.

kgabryje · 2021-06-14T10:32:44Z

@graceguo-supercat Thank you for raising those issues and providing context. PR #15146 solves part of the problem, another part will be fixed in a separate PR by @villebro

…tab (apache#14933) * Optimize finding charts and tabs in scope * Put filters out of scope in Collapse * Use lastFocusedTabId instead of directPathToChild * Fix tests * Fix cypress test * Uncomment e2e test

kgabryje added 3 commits May 31, 2021 17:43

Optimize finding charts and tabs in scope

51b27bb

Put filters out of scope in Collapse

91cada4

Use lastFocusedTabId instead of directPathToChild

6f30ad8

kgabryje added requires:design-review Requires input/approval from a Designer request:qa-review Requests a QA review labels Jun 1, 2021

kgabryje requested a review from villebro June 1, 2021 15:58

pull-request-size Bot added the size/L label Jun 1, 2021

junlincc added release:note demo labels Jun 1, 2021

junlincc removed requires:design-review Requires input/approval from a Designer request:qa-review Requests a QA review labels Jun 2, 2021

villebro reviewed Jun 2, 2021

View reviewed changes

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

Fix tests

28e8c40

kgabryje force-pushed the feat/tabs-native-filters branch from b621f60 to 28e8c40 Compare June 2, 2021 13:11

villebro added preset:2021.17 preset:2021.21 and removed preset:2021.17 preset:2021.21 labels Jun 2, 2021

kgabryje added 2 commits June 2, 2021 16:31

Fix cypress test

1de1426

Uncomment e2e test

c2bd1f5

villebro approved these changes Jun 2, 2021

View reviewed changes

kgabryje merged commit 405f95b into apache:master Jun 2, 2021

villebro mentioned this pull request Jun 3, 2021

fix(native-filters): cascading filters not rendering in tab #14964

Merged

8 tasks

villebro mentioned this pull request Jun 7, 2021

fix: Aligns top level tabs when native filters are enabled #14987

Closed

8 tasks

kgabryje mentioned this pull request Jun 9, 2021

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

Merged

8 tasks

kgabryje requested review from zhaoyongjie and removed request for zhaoyongjie June 9, 2021 14:25

graceguo-supercat reviewed Jun 12, 2021

View reviewed changes

kgabryje mentioned this pull request Jun 14, 2021

fix(native-filters): Don't send unnecessary PUT request on dashboard render #15146

Merged

8 tasks

graceguo-supercat mentioned this pull request Jun 16, 2021

fix: [migration] fix dashboard json_metadata when native filter is not enabled #15197

Closed

8 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dashboard/native-filters): Hide filters out of scope of current tab#14933

feat(dashboard/native-filters): Hide filters out of scope of current tab#14933
kgabryje merged 6 commits into
apache:masterfrom
kgabryje:feat/tabs-native-filters

kgabryje commented Jun 1, 2021 •

edited

Loading

Uh oh!

junlincc commented Jun 2, 2021

Uh oh!

villebro left a comment

Uh oh!

Uh oh!

michael-s-molina commented Jun 2, 2021

Uh oh!

kgabryje commented Jun 2, 2021

Uh oh!

codecov Bot commented Jun 2, 2021 •

edited

Loading

Uh oh!

junlincc commented Jun 2, 2021

Uh oh!

kgabryje commented Jun 2, 2021

Uh oh!

villebro left a comment

Uh oh!

villebro Jun 2, 2021

Uh oh!

graceguo-supercat Jun 12, 2021 •

edited

Loading

Uh oh!

graceguo-supercat commented Jun 13, 2021 •

edited

Loading

Uh oh!

kgabryje commented Jun 14, 2021 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

		xit('should filter by published correctly', () => {
		it('should filter by published correctly', () => {

Conversation

kgabryje commented Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

junlincc commented Jun 2, 2021

Uh oh!

villebro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michael-s-molina commented Jun 2, 2021

Uh oh!

kgabryje commented Jun 2, 2021

Uh oh!

codecov Bot commented Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

junlincc commented Jun 2, 2021

Uh oh!

kgabryje commented Jun 2, 2021

Uh oh!

villebro left a comment

Choose a reason for hiding this comment

Uh oh!

villebro Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

graceguo-supercat Jun 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graceguo-supercat commented Jun 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kgabryje commented Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kgabryje commented Jun 1, 2021 •

edited

Loading

codecov Bot commented Jun 2, 2021 •

edited

Loading

graceguo-supercat Jun 12, 2021 •

edited

Loading

graceguo-supercat commented Jun 13, 2021 •

edited

Loading

kgabryje commented Jun 14, 2021 •

edited

Loading