Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: show spinner on exports #15107

Merged
merged 7 commits into from Jun 12, 2021
Merged

Conversation

betodealmeida
Copy link
Member

@betodealmeida betodealmeida commented Jun 11, 2021

SUMMARY

Show a spinner while an export is being generated.

This is tricky because currently when the user clicks "export" we immediately redirect them to /api/v1/database/export/?q=${rison.encode([database.id])} using window.assign. So while we can set a spinner, we don't know when the download is generated to stop the spinner.

With this PR, in order to stop the spinner the frontend generates a short ID, which is passed to the backend with the export request. Once the export is ready, the backend will set a cookie with the short ID as name. The frontend is constantly polling for that cookie, and when it finds the cookie it hides the spinner.

One problem with this solution is that the GIF stops animating while the export is being generated, since the animation runs on the main thread. To keep the GIF animated the export code injects an iframe, and loads the export there. When the cookie indicating that the download is ready is found, the iframe is deleted.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before, no spinner.

After:

export_spinner.mov

Note: I added an artificial 5 seconds delay in generating the exports for the video, so the spinner can be seen.

TESTING INSTRUCTIONS

Tested with all exports (see screencast). Also tested with the non-card view, though not shown in the video.

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

@codecov
Copy link

codecov bot commented Jun 11, 2021

Codecov Report

Merging #15107 (62285bb) into master (cc2b4fe) will decrease coverage by 0.18%.
The diff coverage is 54.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15107      +/-   ##
==========================================
- Coverage   77.54%   77.35%   -0.19%     
==========================================
  Files         967      968       +1     
  Lines       49752    49880     +128     
  Branches     6351     6374      +23     
==========================================
+ Hits        38578    38583       +5     
- Misses      10972    11095     +123     
  Partials      202      202              
Flag Coverage Δ
hive ?
javascript 72.39% <46.91%> (-0.06%) ⬇️
mysql 81.71% <80.00%> (-0.01%) ⬇️
postgres 81.73% <80.00%> (-0.01%) ⬇️
python 81.82% <80.00%> (-0.30%) ⬇️
sqlite 81.35% <80.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
superset-frontend/src/views/CRUD/utils.tsx 70.58% <ø> (+5.47%) ⬆️
superset-frontend/src/utils/export.ts 23.52% <23.52%> (ø)
...tend/src/views/CRUD/data/database/DatabaseList.tsx 78.07% <44.44%> (-2.31%) ⬇️
.../src/views/CRUD/data/savedquery/SavedQueryList.tsx 72.91% <50.00%> (-1.72%) ⬇️
...set-frontend/src/views/CRUD/welcome/ChartTable.tsx 74.62% <50.00%> (-3.34%) ⬇️
...frontend/src/views/CRUD/welcome/DashboardTable.tsx 64.28% <50.00%> (-1.51%) ⬇️
...perset-frontend/src/views/CRUD/chart/ChartList.tsx 72.66% <55.55%> (-1.10%) ⬇️
...rontend/src/views/CRUD/dashboard/DashboardList.tsx 74.62% <55.55%> (-1.38%) ⬇️
...ontend/src/views/CRUD/data/dataset/DatasetList.tsx 70.05% <55.55%> (-0.13%) ⬇️
superset/charts/api.py 86.44% <80.00%> (-0.14%) ⬇️
... and 22 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 cc2b4fe...62285bb. Read the comment docs.

Copy link
Member

@eschutho eschutho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM I just had the one nit about type checking.

@betodealmeida betodealmeida added the need:merge The PR is ready to be merged label Jun 11, 2021
@betodealmeida betodealmeida merged commit 53df152 into apache:master Jun 12, 2021
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
* feat: show spinner on exports

* Set cookie only if token is passed

* Use iframe

* Small fixes

* Fix lint

* Remove stale test

* Add explicit type
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
* feat: show spinner on exports

* Set cookie only if token is passed

* Use iframe

* Small fixes

* Fix lint

* Remove stale test

* Add explicit type
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
* feat: show spinner on exports

* Set cookie only if token is passed

* Use iframe

* Small fixes

* Fix lint

* Remove stale test

* Add explicit type
@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels need:merge The PR is ready to be merged size/L 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants