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.

Sign up for GitHub

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

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

Merged
merged 4 commits into from
Jan 7, 2021
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ada2c49
Update svg-crowbar to fix error
richardwestenra Dec 18, 2020
ae28ef5
Update svg-crowbar to 0.6.5
richardwestenra Jan 7, 2021
9c1680b
Prevent console errors when exporting null graph
richardwestenra Jan 7, 2021
3e52aa4
Check for finite values on graphSize
richardwestenra Jan 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
"reselect": "^4.0.0",
"seedrandom": "^3.0.5",
"snyk": "^1.369.2",
"svg-crowbar": "^0.6.0",
"svg-crowbar": "^0.6.5",
"what-input": "^5.2.10"
},
"peerDependencies": {
Expand Down Expand Up @@ -121,4 +121,4 @@
"not op_mini all"
],
"snyk": true
}
}
20 changes: 13 additions & 7 deletions src/components/export-modal/export-graph.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,13 @@ const exportGraph = ({ format, theme, graphSize, mockFn }) => {
clone.classList.add('kedro', `kui-theme--${theme}`, 'pipeline-graph--export');

// Reset zoom/translate
let width = graphSize.width + graphSize.marginx * 2;
let height = graphSize.height + graphSize.marginy * 2;
clone.setAttribute('viewBox', `0 0 ${width} ${height}`);
let width, height;
const hasGraph = graphSize.width && graphSize.height;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point! fixed 👍

if (hasGraph) {
width = graphSize.width + graphSize.marginx * 2;
height = graphSize.height + graphSize.marginy * 2;
clone.setAttribute('viewBox', `0 0 ${width} ${height}`);
}
clone.querySelector('#zoom-wrapper').removeAttribute('transform');

// Impose a maximum size on PNGs because otherwise they break when downloading
Expand All @@ -32,8 +36,10 @@ const exportGraph = ({ format, theme, graphSize, mockFn }) => {
width = Math.min(width, maxWidth);
height = Math.min(height, maxWidth * (height / width));
}
clone.setAttribute('width', width);
clone.setAttribute('height', height);
if (hasGraph) {
clone.setAttribute('width', width);
clone.setAttribute('height', height);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


const style = document.createElement('style');
if (format === 'svg') {
Expand All @@ -50,8 +56,8 @@ const exportGraph = ({ format, theme, graphSize, mockFn }) => {
clone.prepend(style);

// Download SVG/PNG
download(clone, 'kedro-pipeline');
// @TODO: Replace third { css: 'internal' } argument when CORS issue is fixed
const options = format === 'svg' ? { css: 'internal' } : undefined;
download(clone, 'kedro-pipeline', options);

// Delete cloned SVG
svg.parentNode.removeChild(clone);
Expand Down