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

Set SVG viewBox size from canvas width and height without factoring by devicePixelDensity #411

Merged
merged 1 commit into from
Jun 15, 2021

Conversation

sampriddy
Copy link
Contributor

@sampriddy sampriddy commented Dec 6, 2018

There is currently an issue in the export of SVGs that causes data to be truncated off the right-hand and bottom.

The problem lies in the fact that stroke data is being recorded in terms of the overall size of the canvas and then the size of the viewBox is being reduced afterwards. It's not necessary to reduce the viewBox's size because the viewBox's width and height are unitless and only meaningful in relation to the stroke data. Reducing the maxX of the viewBox from 1000 to 500, for example, just invalidates coordinates with x > 500 because they no longer exist in the viewBox.

I think the intent of this originally must have been to preserve the dimensions of the image across devices of differing DPI, and for that we only need to adjust the width and height attributes.

@sampriddy sampriddy changed the title Set viewBox size from canvas width and height without factoring by devicePixelDensity Set SVG viewBox size from canvas width and height without factoring by devicePixelDensity Dec 6, 2018
@rickychilcott
Copy link

This checks out. Please merge.

@AugmentBLU
Copy link

This works for me as well, will it be merged?

@mesqueeb
Copy link

I need this PR to be merged and pushed to NPM as well.
I'm currently still using this library from my own fork. 😰

@abraxxa
Copy link

abraxxa commented Sep 15, 2019

We'd need that fix released too. What's blocking it?

@prestontighe
Copy link

+1

sdahlbac added a commit to ecraft/signature_pad that referenced this pull request Jan 8, 2020
@abraxxa abraxxa mentioned this pull request Mar 15, 2020
@patrickbussmann
Copy link
Contributor

Funny 😂
Because I had many troubles with this, too. I already fixed it by myself.
And now I wanted to make a PR and found your's 🤣

var svgStr = atob(signaturePad.toDataURL('image/svg+xml').replace(/^[^,]+,/, ''));
svgStr = svgStr.replace(/(viewBox)="[^"]+"/i, '$1="0 0 ' + canvas.width + ' ' + canvas.height + '"');
svgStr = svgStr.replace(/(<svg[^>]+width)="[^"]+"/i, '$1="' + canvas.width + '"');
svgStr = svgStr.replace(/(<svg[^>]+height)="[^"]+"/i, '$1="' + canvas.height + '"');
return 'data:image/svg+xml;base64,' + btoa(svgStr);

@szimek can this be merged soon?
Thanks in Advance 👍

@UziTech UziTech added the bug label Jun 8, 2021
@szimek
Copy link
Owner

szimek commented Jun 13, 2021

@UziTech Would you have time to take a look at it? If not, I'll take a look at it myself next weekend.

@szimek szimek added this to the 3.0 final release milestone Jun 13, 2021
@UziTech
Copy link
Collaborator

UziTech commented Jun 13, 2021

I can look at it later this week I'll be gone the next few days with limited access to internet.

@UziTech
Copy link
Collaborator

UziTech commented Jun 15, 2021

@szimek I think this should be merged. The viewBox is unitless so devicePixelRatio should not be considered.

@merbin2012
Copy link

Funny 😂 Because I had many troubles with this, too. I already fixed it by myself. And now I wanted to make a PR and found your's 🤣

var svgStr = atob(signaturePad.toDataURL('image/svg+xml').replace(/^[^,]+,/, ''));
svgStr = svgStr.replace(/(viewBox)="[^"]+"/i, '$1="0 0 ' + canvas.width + ' ' + canvas.height + '"');
svgStr = svgStr.replace(/(<svg[^>]+width)="[^"]+"/i, '$1="' + canvas.width + '"');
svgStr = svgStr.replace(/(<svg[^>]+height)="[^"]+"/i, '$1="' + canvas.height + '"');
return 'data:image/svg+xml;base64,' + btoa(svgStr);

@szimek can this be merged soon? Thanks in Advance 👍

Thank you very much, it is working.

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

Successfully merging this pull request may close these issues.

10 participants