[KED-2355] Update SVG-Crowbar to fix errors #339

richardwestenra · 2020-12-18T15:25:11Z

Description

We were originally using SVG-Crowbar v0.6.0, but started experiencing a critical CORS error with internal CSS in exported images, which was patched in #337. This PR updates the dependency version and reverts the patch.

Development notes

SVG-Crowbar v0.6.3 fixes the CORS issue, so we can revert the fix in #337.
However v0.6.2 introduced a call stack size error when exporting PNGs, which was resolved in v0.6.4.
However, v0.6.4 introduced an issue with PNG cropping that has been resolved in 0.6.5.
So in summary, we have now updated to v0.6.5 now that all of these errors have been resolved.

I've also added a small fix for console errors when exporting a null graph. If the graph doesn't exist and graphSize is empty, which can happen if the graph is still loading/rendering, or if all the filters are applied, then NaN console errors will be thrown. This commit prevents these errors.

QA notes

You will need to update/install the SVG-Crowbar dependency to test this fix.

Checklist

Read the contributing guidelines
Opened this PR as a 'Draft Pull Request' if it is work-in-progress
Updated the documentation to reflect the code changes
Added new entries to the RELEASE.md file
Added tests to cover my changes

Legal notice

I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":
I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.
I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

SVG-Crowbar v0.6.3 fixes a CORS issue that we were facing, so we can revert the fix in #337. However v0.6.2 seems to have introduced a new issue with PNG exports, so we cannot merge this change yet until it's fixed.

studioswong

Thanks for the quick fix!

richardwestenra · 2020-12-18T15:41:01Z

@studioswong Thanks, but this isn't fixed yet, due to the new bug in SVG-Crowbar ~v0.6.2! We won't be able to update it to merge this PR and properly resolve this issue until that bug is fixed. Hence this is just a draft PR, and I'm gonna leave it here for when I pick this back up after xmas.

v0.6.2 introduced [a call stack size error](cy6erskunk/svg-crowbar#278) when exporting PNGs, which was resolved in v0.6.4. However, v0.6.4 introduced [an issue with PNG cropping](cy6erskunk/svg-crowbar#75) that has been resolved in 0.6.5.

If the graph doesn't exist and graphSize is empty, which can happen if the graph is still loading/rendering, or if all the filters are applied, then NaN console errors will be thrown. This commit prevents these errors.

studioswong · 2021-01-07T14:37:12Z

Thanks for the fix @richardwestenra , and great work for keeping a close eye and enduring all the SVG crowbar saga 👍

I just checked out the branch and both exports (SVG and PNG) works well 👍

Tried exporting the graph with all filters applied, while the export works fine ( it just exports an empty image with your theme background), I noticed that the console still throws errors for NaN and Infinity values ( see below). I guess we would need to apply a check for those x and y values as well to eliminate those errors?

Perhaps we might even need to rethink about those situations with the empty flowchart, perhaps we might even grey out the export function given these type of situations there is no graph?

All in all, thanks for the work!

studioswong · 2021-01-07T14:40:43Z

src/components/export-modal/export-graph.js

-  let height = graphSize.height + graphSize.marginy * 2;
-  clone.setAttribute('viewBox', `0 0 ${width} ${height}`);
+  let width, height;
+  const hasGraph = graphSize.width && graphSize.height;


We might need to add the checks for valid values of width and height to eliminate those console errors?

good point! fixed 👍

studioswong · 2021-01-07T14:43:02Z

src/components/export-modal/export-graph.js

+  if (hasGraph) {
+    clone.setAttribute('width', width);
+    clone.setAttribute('height', height);
+  }


it might be worth to rethink about the else scenerio for the empty flowchart - we could perhaps apply a standard width and height in the meantime for the sake of allowing us to still export and empty graph while we look into the flow when exporting an empty graph?

I think it works as is without needing an explict height - I don't think we need to think any more about this extreme edge case. I tidied up the console errors for the sake of completeness as I don't like exposing uncaught errors, but I don't think it needs anything else tbh.

sure thing, just tested and the errors are gone, so if it works without the need to manually specify a width and height then it's all good.

# Release 3.9.0 ## Major features and improvements - Add code panel (#346) - Improve view panning behaviour when a node is selected (#356, #363, #370, #373, #374) - Improve layout performance for large graphs (#343) - Save tag state to localStorage (#353) ## Bug fixes and other changes - Improve graph layout code quality, performance and docs (#347) - Update docs to remind on compatibility of Kedro-Viz 3.8.0 with Kedro 0.17 (#367) - Update dependencies (#360, #364, #369) - Fix failing CircleCI build on Windows (#365, #366) - Refactor node-list-row CSS, fixing hover and focus states (#355, #358, #362) - Update iconography (#357, #359) - Fix missing indicator Chrome zoom bug (#349) - Fix sidebar border/box-shadow CSS rules (#351) - Fix server.py to work with versions >0.17 and update contributing docs (#348) - Fix errors when scrolling with empty pipeline (#342) - Ignore coverage on some branches and fix e2e tests (#345) - Fix icon-button tooltips on mobile (#344) - Update SVG-Crowbar to fix errors (#339) - Update contributing docs for new dev server (#341)

Update svg-crowbar to fix error

ada2c49

SVG-Crowbar v0.6.3 fixes a CORS issue that we were facing, so we can revert the fix in #337. However v0.6.2 seems to have introduced a new issue with PNG exports, so we cannot merge this change yet until it's fixed.

richardwestenra added the Type: Fix label Dec 18, 2020

richardwestenra self-assigned this Dec 18, 2020

studioswong approved these changes Dec 18, 2020

View reviewed changes

richardwestenra added 2 commits January 7, 2021 12:33

Update svg-crowbar to 0.6.5

ae28ef5

v0.6.2 introduced [a call stack size error](cy6erskunk/svg-crowbar#278) when exporting PNGs, which was resolved in v0.6.4. However, v0.6.4 introduced [an issue with PNG cropping](cy6erskunk/svg-crowbar#75) that has been resolved in 0.6.5.

Prevent console errors when exporting null graph

9c1680b

If the graph doesn't exist and graphSize is empty, which can happen if the graph is still loading/rendering, or if all the filters are applied, then NaN console errors will be thrown. This commit prevents these errors.

richardwestenra marked this pull request as ready for review January 7, 2021 12:42

richardwestenra requested a review from studioswong January 7, 2021 12:43

studioswong reviewed Jan 7, 2021

View reviewed changes

Check for finite values on graphSize

3e52aa4

studioswong reviewed Jan 7, 2021

View reviewed changes

richardwestenra changed the title ~~Update svg-crowbar to fix error~~ Update SVG-Crowbar to fix errors Jan 7, 2021

richardwestenra merged commit 983b5d8 into main Jan 7, 2021

richardwestenra deleted the fix/export-css-rules2 branch January 7, 2021 15:07

richardwestenra changed the title ~~Update SVG-Crowbar to fix errors~~ [KED-2355] Update SVG-Crowbar to fix errors Jan 7, 2021

richardwestenra mentioned this pull request Feb 17, 2021

3.9.0 #371

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KED-2355] Update SVG-Crowbar to fix errors #339

[KED-2355] Update SVG-Crowbar to fix errors #339

richardwestenra commented Dec 18, 2020 •

edited

Loading

studioswong left a comment

richardwestenra commented Dec 18, 2020 •

edited

Loading

studioswong commented Jan 7, 2021 •

edited

Loading

studioswong Jan 7, 2021

richardwestenra Jan 7, 2021

studioswong Jan 7, 2021

richardwestenra Jan 7, 2021

studioswong Jan 7, 2021

[KED-2355] Update SVG-Crowbar to fix errors #339

[KED-2355] Update SVG-Crowbar to fix errors #339

Conversation

richardwestenra commented Dec 18, 2020 • edited Loading

Description

Development notes

QA notes

Checklist

Legal notice

studioswong left a comment

Choose a reason for hiding this comment

richardwestenra commented Dec 18, 2020 • edited Loading

studioswong commented Jan 7, 2021 • edited Loading

studioswong Jan 7, 2021

Choose a reason for hiding this comment

richardwestenra Jan 7, 2021

Choose a reason for hiding this comment

studioswong Jan 7, 2021

Choose a reason for hiding this comment

richardwestenra Jan 7, 2021

Choose a reason for hiding this comment

studioswong Jan 7, 2021

Choose a reason for hiding this comment

richardwestenra commented Dec 18, 2020 •

edited

Loading

richardwestenra commented Dec 18, 2020 •

edited

Loading

studioswong commented Jan 7, 2021 •

edited

Loading