Skip to content

Conversation

@kgabryje
Copy link
Member

SUMMARY

Due to Mapbox controls being loaded from different origin, it was impossible to convert Mapbox canvas to base64 encoded image due to CORS policy of HTMLCanvasElement.toDataURL method (https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/toDataURL#exceptions). This PR fixes it by filtering out div that contains control elements (such as Mapbox logo).
This PR needs apache-superset/superset-ui-plugins-deckgl#25 and apache-superset/superset-ui#965 to be merged and respective packages to be bumped to work properly. Mapbox and DeckGL components need to have preserveDrawingBuffer prop set to true - otherwise only a gray background is getting captured. This change may affect performance of MapBox and DeckGL charts - I'd appreciate some stress tests and verifying if performance is acceptable when compared to current version

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before: see #12745
After:
country-of-citizenship-2021-02-17T17-01-42 411Z
country-of-citizenship-2021-02-17T17-02-26 826Z

TEST PLAN

ADDITIONAL INFORMATION

CC: @villebro @junlincc @altef

@villebro
Copy link
Member

As the dependent PRs are probably best tested and merged in one go, I can review and test this tomorrow morning CET unless someone beats me to it.

@junlincc junlincc requested a review from ktmud February 17, 2021 20:17
@kgabryje kgabryje force-pushed the fix/download-map-as-image branch from 5c55c84 to ecf710d Compare February 18, 2021 08:40
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Tested along with the accompanying PRs - I can confirm that all charts download nicely as images, and I wasn't able to see any noticeable performance degradation due to this. Unless someone has any objections I propose merging the superset-ui PRs, bumping the package versions here and merging this.

@villebro
Copy link
Member

@kgabryje the dependent PRs have now been merged and deployed (btw, this also needs a rebase). The 0.17.10 release of superset-ui contains lots of unrelated fixes, so not sure which apache/superset PR should bump those (I'd almost vote for keeping that separate from these to avoid confusion). If this PR doesn't explicitly require the bump on superset-ui, I'd recommend just bumping the DeckGL plugin to the latest version and letting the rest of the bumps roll in through a separate PR in due time.

@kgabryje kgabryje force-pushed the fix/download-map-as-image branch from ecf710d to 1cc2497 Compare February 20, 2021 10:35
@kgabryje
Copy link
Member Author

@villebro Done. I bumped only deckgl as you suggested. We can put off bumping superset-ui, but until we do, only a gray background will get downloaded when user tries to save Mapbox chart as image (before the fix, it just failed silently and nothing got downloaded).

@codecov-io
Copy link

codecov-io commented Feb 20, 2021

Codecov Report

Merging #13181 (1cc2497) into master (2ce7982) will increase coverage by 19.80%.
The diff coverage is 62.42%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #13181       +/-   ##
===========================================
+ Coverage   53.06%   72.86%   +19.80%     
===========================================
  Files         489      555       +66     
  Lines       17314    20544     +3230     
  Branches     4482     5377      +895     
===========================================
+ Hits         9187    14969     +5782     
+ Misses       8127     5447     -2680     
- Partials        0      128      +128     
Flag Coverage Δ
cypress 58.45% <66.00%> (+5.39%) ⬆️
javascript 62.11% <47.50%> (?)

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

Impacted Files Coverage Δ
...end/src/SqlLab/components/RunQueryActionButton.tsx 64.28% <ø> (+11.50%) ⬆️
...erset-frontend/src/SqlLab/components/SqlEditor.jsx 56.27% <ø> (+1.81%) ⬆️
...end/src/SqlLab/components/TemplateParamsEditor.jsx 23.80% <ø> (ø)
superset-frontend/src/chart/ChartContainer.jsx 100.00% <ø> (ø)
superset-frontend/src/chart/ChartRenderer.jsx 77.02% <0.00%> (+0.31%) ⬆️
...perset-frontend/src/common/components/Dropdown.tsx 54.76% <ø> (+4.76%) ⬆️
.../src/common/components/Tooltip/Tooltip.stories.tsx 0.00% <0.00%> (ø)
...t-frontend/src/common/components/Tooltip/index.tsx 100.00% <ø> (ø)
...perset-frontend/src/components/AlteredSliceTag.jsx 98.66% <ø> (+6.66%) ⬆️
superset-frontend/src/components/CachedLabel.jsx 42.10% <ø> (ø)
... and 548 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 786c12d...1cc2497. Read the comment docs.

@villebro
Copy link
Member

Thanks @kgabryje ! Let me open up a bump PR so we can get these in asap.

@villebro villebro merged commit c1aacde into apache:master Feb 20, 2021
@junlincc
Copy link
Member

@villebro I am gonna do a round of QA in master once you are done with all merging😃 thank you Ville!

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 First shipped in 1.2.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 size/M viz:charts:deck.gl Related to deck.gl charts 🚢 1.2.0 First shipped in 1.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants