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

Remove download svg option due to highcharts limitations #1498

Merged
merged 3 commits into from
Jun 14, 2022

Conversation

tongyu-microsoft
Copy link
Contributor

@tongyu-microsoft tongyu-microsoft commented Jun 14, 2022

This PR removes "Download svg vector image" option from highchart exporting menu list.
The issue is, downloaded svg is corrupt and can't view it.
image
The root cause is, we adds the highcharts accessibility module to the imported Highcharts namespace by Accessibility.default(Highcharts);. And this module adds a div element to the svg file that we are trying to download (see the red underline in screenshot below). So the svg file fails to be rendered. If we remove Accessibility.default(Highcharts);, then the svg element can be rendered in browser. But we want to enable accessibility for highcharts. So we remove "Download svg vector image" option from highchart exporting menu list.
image

The default exporting menu list is ["viewFullscreen", "printChart", "separator", "downloadPNG", "downloadJPEG", "downloadPDF", "downloadSVG"], we remove the "downloadSVG", so this PR specify menuItems to be ["viewFullscreen", "printChart", "separator", "downloadPNG", "downloadJPEG", "downloadPDF"].

Description

Before:
image

After:
image

Checklist

  • I have added screenshots above for all UI changes.
  • I have added e2e tests for all UI changes.
  • Documentation was updated if it was needed.

@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2022

Codecov Report

Merging #1498 (ee46941) into main (2e339ed) will decrease coverage by 2.34%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1498      +/-   ##
==========================================
- Coverage   89.91%   87.57%   -2.35%     
==========================================
  Files          90      110      +20     
  Lines        3848     5093    +1245     
==========================================
+ Hits         3460     4460    +1000     
- Misses        388      633     +245     
Flag Coverage Δ
unittests 87.57% <ø> (-2.35%) ⬇️

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

Impacted Files Coverage Δ
raiwidgets/raiwidgets/error_handling.py 100.00% <0.00%> (ø)
raiwidgets/raiwidgets/__init__.py 100.00% <0.00%> (ø)
raiwidgets/raiwidgets/responsibleai_dashboard.py 67.85% <0.00%> (ø)
raiwidgets/raiwidgets/constants.py 99.40% <0.00%> (ø)
raiwidgets/raiwidgets/explanation_dashboard.py 83.33% <0.00%> (ø)
...iwidgets/raiwidgets/explanation_dashboard_input.py 60.13% <0.00%> (ø)
raiwidgets/raiwidgets/cohort.py 95.00% <0.00%> (ø)
raiwidgets/raiwidgets/utils.py 100.00% <0.00%> (ø)
raiwidgets/raiwidgets/fairness_dashboard.py 19.56% <0.00%> (ø)
raiwidgets/raiwidgets/dashboard.py 92.68% <0.00%> (ø)
... and 10 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 2e339ed...ee46941. Read the comment docs.

1 similar comment
Copy link
Contributor

@romanlutz romanlutz left a comment

Choose a reason for hiding this comment

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

This is a great find! Is this a known bug in highcharts? If not, should we file it?

No objection to your change, of course!

@tongyu-microsoft tongyu-microsoft enabled auto-merge (squash) June 14, 2022 15:48
@tongyu-microsoft
Copy link
Contributor Author

This is a great find! Is this a known bug in highcharts? If not, should we file it?

No objection to your change, of course!

@romanlutz Yes! We have filed a bug to highcharts: highcharts/highcharts#17390

@tongyu-microsoft tongyu-microsoft merged commit 584228c into main Jun 14, 2022
@tongyu-microsoft tongyu-microsoft deleted the tongy/removeSVG branch June 14, 2022 16:53
@tongyu-microsoft
Copy link
Contributor Author

A workaround: https://jsfiddle.net/oysteinmoseng/yezu6nx3/
The latest version of Highcharts has the fix for this svg export issue: highcharts/highcharts#16589

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants