perf(native-filters): Load native filters after charts #14443

simcha90 · 2021-05-02T14:09:37Z

SUMMARY

Show filter bar after all charts loaded

Dependent on: #14408

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-05-04.at.9.17.02.mov

TEST PLAN

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

� Conflicts: � superset-frontend/src/dashboard/util/getPermissions.ts

� Conflicts: � superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx

villebro · 2021-05-03T05:54:56Z

Reviewing/testing #14408 first, but one problem with this approach is that it will cause two renders of charts - first with the full width of the dashboard without the filter tab, and next with the filter tab. To address this I would recommend making the filter tab visible initially, but potentially introduce the loading spinner instead of the tab contents to highlight that filters are still pending.

� Conflicts: � superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx � superset-frontend/src/dashboard/components/nativeFilters/FilterBar/state.ts � superset-frontend/src/dashboard/components/nativeFilters/FilterBar/utils.ts � superset-frontend/src/dashboard/types.ts � superset-frontend/src/dataMask/reducer.ts

codecov · 2021-05-04T11:35:00Z

Codecov Report

Merging #14443 (3596366) into master (2f9efb2) will increase coverage by 0.13%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master   #14443      +/-   ##
==========================================
+ Coverage   76.95%   77.09%   +0.13%     
==========================================
  Files         955      957       +2     
  Lines       48091    48141      +50     
  Branches     6036     6056      +20     
==========================================
+ Hits        37008    37112     +104     
+ Misses      10884    10828      -56     
- Partials      199      201       +2

Flag	Coverage Δ
javascript	`71.91% <88.88%> (+0.30%)`	⬆️

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

Impacted Files	Coverage Δ
superset-frontend/src/chart/Chart.jsx	`50.00% <ø> (ø)`
...d/components/DashboardBuilder/DashboardBuilder.tsx	`86.95% <ø> (ø)`
...hboard/components/nativeFilters/FilterBar/state.ts	`88.63% <86.36%> (-2.67%)`	⬇️
...board/components/nativeFilters/FilterBar/index.tsx	`92.94% <100.00%> (+1.58%)`	⬆️
.../FilterBar/CascadeFilters/CascadePopover/index.tsx	`63.38% <0.00%> (-1.41%)`	⬇️
...et-frontend/src/views/CRUD/data/database/styles.ts
...end/src/views/CRUD/data/database/DatabaseModal.tsx
.../CRUD/data/database/DatabaseModal/ExtraOptions.tsx	`93.33% <0.00%> (ø)`
...c/views/CRUD/data/database/DatabaseModal/styles.ts	`96.00% <0.00%> (ø)`
...c/views/CRUD/data/database/DatabaseModal/index.tsx	`59.64% <0.00%> (ø)`
... and 7 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 2f9efb2...3596366. Read the comment docs.

villebro

One small comment, other than that LGTM and was verified to work well in all following cases:

regular examples dashboard
dashboard with no charts on first page
Global Async Query framework (websocket)

villebro · 2021-05-04T12:35:37Z

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

+        {!isInitialized ? (
+          <Loading />
+        ) : isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS_SET) ? (


I think this causes an unnecessary visual artefact - I think it's ok to keep this as-is, i.e. showing the filters in spinning state until they've fully resolved.

but this is the point not to render them in order they will not start to do requests to the server :)

Haha, makes sense 🤦

villebro

LGTM

* fix:fix get permission function * refactor: filter default value * refactor: update default value loading * refactor: apply defaultValues * lint: fix lint * lint: fix lint * test: fix test * refactor: use extraFormData for reload charts * feat: load native filters after after charts * feat: load filters after charts * fix: revert changes * test: fix timers * test: fix tests (cherry picked from commit 582900c)

* fix:fix get permission function * refactor: filter default value * refactor: update default value loading * refactor: apply defaultValues * lint: fix lint * lint: fix lint * test: fix test * refactor: use extraFormData for reload charts * feat: load native filters after after charts * feat: load filters after charts * fix: revert changes * test: fix timers * test: fix tests

* fix:fix get permission function * refactor: filter default value * refactor: update default value loading * refactor: apply defaultValues * lint: fix lint * lint: fix lint * test: fix test * refactor: use extraFormData for reload charts * feat: load native filters after after charts * feat: load filters after charts * fix: revert changes * test: fix timers * test: fix tests (cherry picked from commit 582900c)

* fix:fix get permission function * refactor: filter default value * refactor: update default value loading * refactor: apply defaultValues * lint: fix lint * lint: fix lint * test: fix test * refactor: use extraFormData for reload charts * feat: load native filters after after charts * feat: load filters after charts * fix: revert changes * test: fix timers * test: fix tests

simcha90 added 16 commits April 13, 2021 16:04

fix:fix get permission function

6eb04b1

Merge branch 'master' of github.com:apache/superset

5e9a1d8

� Conflicts: � superset-frontend/src/dashboard/util/getPermissions.ts

Merge branch 'master' of github.com:apache/superset

2c5800a

Merge branch 'master' of github.com:apache/superset

ea9aa8b

Merge branch 'master' of github.com:apache/superset

402918a

Merge branch 'master' of github.com:apache/superset

1a3cc39

refactor: filter default value

1b79e99

refactor: update default value loading

1995881

Merge branch 'master' of github.com:apache/superset into native_default

404b538

refactor: apply defaultValues

411a6c4

Merge branch 'master' of github.com:apache/superset into native_default

d332ebd

� Conflicts: � superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx

lint: fix lint

3671225

lint: fix lint

8b3f300

test: fix test

fba94fd

refactor: use extraFormData for reload charts

c282535

feat: load native filters after after charts

7730567

pull-request-size bot added the size/L label May 2, 2021

amitmiran137 added the v1.2 label May 2, 2021

villebro mentioned this pull request May 3, 2021

[native-filter][performance] Dashboard load slower with native-filter components #14421

Closed

3 tasks

villebro changed the title ~~WIP: feat(native-filters): Load native filters after charts~~ WIP: perf(native-filters): Load native filters after charts May 3, 2021

pull-request-size bot added size/S and removed size/L labels May 3, 2021

junlincc requested a review from villebro May 3, 2021 20:11

feat: load filters after charts

1b8eca5

pull-request-size bot added size/M and removed size/S labels May 4, 2021

simcha90 changed the title ~~WIP: perf(native-filters): Load native filters after charts~~ perf(native-filters): Load native filters after charts May 4, 2021

fix: revert changes

6422ee1

simcha90 added 2 commits May 4, 2021 11:30

test: fix timers

13c6948

test: fix tests

3596366

villebro reviewed May 4, 2021

View reviewed changes

villebro approved these changes May 4, 2021

View reviewed changes

villebro merged commit 582900c into apache:master May 4, 2021

junlincc added the dashboard:native-filters Related to the native filters of the Dashboard label May 12, 2021

mistercrunch added 🍒 1.2.0 🏷️ 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

perf(native-filters): Load native filters after charts #14443

perf(native-filters): Load native filters after charts #14443

simcha90 commented May 2, 2021 •

edited

villebro commented May 3, 2021

codecov bot commented May 4, 2021

villebro left a comment

villebro May 4, 2021

simcha90 May 4, 2021 •

edited

villebro May 4, 2021

villebro left a comment

perf(native-filters): Load native filters after charts #14443

perf(native-filters): Load native filters after charts #14443

Conversation

simcha90 commented May 2, 2021 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

villebro commented May 3, 2021

codecov bot commented May 4, 2021

Codecov Report

villebro left a comment

Choose a reason for hiding this comment

villebro May 4, 2021

Choose a reason for hiding this comment

simcha90 May 4, 2021 • edited

Choose a reason for hiding this comment

villebro May 4, 2021

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

simcha90 commented May 2, 2021 •

edited

simcha90 May 4, 2021 •

edited