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

Add option to adjust the viewbox to match the width of the svg #43

Open
wants to merge 3 commits into
base: gh-pages
Choose a base branch
from

Conversation

xchrdw
Copy link
Contributor

@xchrdw xchrdw commented Jul 6, 2023

I added an option to automatically shift and translate icons which have a viewbox that doesn't start at 0,0 and doesn't match the width of the svg.

@@ -46,9 +46,10 @@ <h1>Skiafy</h1>
<br/>
<span class="limitation">Works only with fill attributes & expects colors in form #FFF or #FFFFFF.</span>
</label>
<label><input id="fix-viewbox" type="checkbox"> Translate and scale the viewbox to match the width of the svg</label><br/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I enable this checkbox by default? We usually don't get the right result if the width/height of the svg doesn't match the viewbox. If it matches, this checkbox is a no-op

Copy link
Owner

Choose a reason for hiding this comment

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

Only thing I'm worried about is how this breaks in cases we haven't considered like a non-square size, or unspecified size, or non-square viewbox. Therefore it might be safer to not check it by default.

Also can we update the text to

"""
Normalize viewbox and path coordinates to match the specified width of the SVG (assuming square dimensions).
"""

if (canvasSize != 48)
output += 'CANVAS_DIMENSIONS, ' + canvasSize + ',\n';

Copy link
Owner

Choose a reason for hiding this comment

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

nit: remove spaces

var output = '';
var canvasSize = svgNode.viewBox.baseVal.width;
if (canvasSize == 0)
canvasSize = svgNode.width.baseVal.value;

if (fixViewbox) {
Copy link
Owner

Choose a reason for hiding this comment

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

nit: document what's happening/what this is for


var output = ProcessSvg(svgAnchor.querySelector('svg'), 1, 1, 0, 0, preserveFill);
var output = ProcessSvg(svgAnchor.querySelector('svg'), 1, 1, 0, 0, preserveFill, fixViewbox);
Copy link
Owner

Choose a reason for hiding this comment

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

nit: maybe rather than fixViewbox everywhere it should be normalizeCoordinates? You're changing all the coordinates in the paths, not just the viewbox.

var output = '';
var canvasSize = svgNode.viewBox.baseVal.width;
if (canvasSize == 0)
canvasSize = svgNode.width.baseVal.value;

if (fixViewbox) {
var scaleFactor = svgNode.width.baseVal.value / svgNode.viewBox.baseVal.width;
Copy link
Owner

Choose a reason for hiding this comment

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

what if there is no width specified?

@@ -46,9 +46,10 @@ <h1>Skiafy</h1>
<br/>
<span class="limitation">Works only with fill attributes & expects colors in form #FFF or #FFFFFF.</span>
</label>
<label><input id="fix-viewbox" type="checkbox"> Translate and scale the viewbox to match the width of the svg</label><br/>
Copy link
Owner

Choose a reason for hiding this comment

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

Only thing I'm worried about is how this breaks in cases we haven't considered like a non-square size, or unspecified size, or non-square viewbox. Therefore it might be safer to not check it by default.

Also can we update the text to

"""
Normalize viewbox and path coordinates to match the specified width of the SVG (assuming square dimensions).
"""

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.

2 participants