Does it need to get SVG height?

[akkunchoi]

I try to remove follow lines, and it works.

// iconfont-webpack-plugin/lib/icons-to-woff.js line 52.

const iconHeight = getSvgHeight(result, filename);
if (!iconHeight) {
  return reject(new Error(`SVG font generation failed as icon "${filename}" does not have a height.`));
}

// iconfont-webpack-plugin/lib/icons-to-woff.js line 69.

/**
 * Reads the height of the svg
 *
 * @param  {String} svg      the svg content
 * @param  {String} filename the file name for error reporting
 * @return {Number}          height
 */
function getSvgHeight (svg, filename) {
  const parseSvg = /<svg[^>]+height\s*=\s*["']?(\d+)\s*(pt|px|)["']?/i.exec(svg);
  if (!parseSvg) {
    throw new Error(`could not read height for '${filename}'.`);
  }
  return parseSvg[1];
}

Without height property in svg is more useful for me. Because Illustrator outputs svgs without height property.

[jantimon]

There used to be bugs if svgs didn’t have the same height - I don’t know if those still exists.. You should probably always provide a height otherwise the proportion of icons to each other will be wrong once they are turned into font glyphs

[akkunchoi]

Thanks for your reply.

I also used to gulp-iconfont which is not required svg height. I am pleased if height is option.

[Neunerlei]

I tried to implement your plugin into our workflow. Generally speaking it works great! But the issue with the height is kind of irritating.

As a suggestion: You could make it somewhat more robust if you parse the viewBox (set by Adobe Illustrator and Photoshop) as a fallback, or let us supply an option which can be used to set a default height. Killing webpack by throwing an error is kind of drastic (I would suggest this.emitError / this.emitWarning in your loader).

I also tried to create an svg webpack preloader, but found, that your plugin sadly does not utilize https://webpack.js.org/api/loaders#thisloadmodule to load the svg, but the fs directly, which is a bummer, as it prevents other loaders to supply the height attribute on the fly :(.

if(!parseSvg){
    // Check if we can read the viewbox to extract the height
    // Thanks to https://github.com/Finesse for the suggestion for negative values and multiple spaces
        const viewbox = /<svg\s[^>]*viewBox\s*=\s*["']?\s*[\d.\-]+\s+[\d.\-]+\s+([\d.\-]+)\s+([\d.\-]+)\s*["']?/im.exec(source);
    const height = viewbox && viewbox[2];
        if(!height) // Fail here...
}

Otherwise, great plugin! Thanks for sharing it.

Cheers!

[jantimon]

@Neunerlei if it works with this.loadmodule we should definitely change it - also the error method you are suggesting is way better.

The height limitation came from the underlying library which converted svg in fonts.

[Neunerlei]

@jantimon this.loadmodule: It should work... theoretically... I did not test it for myself yet; so please don't hit mit if it doesn't 🤷‍♂️

Just to let you know: I wrote a temporary, dirty hack which implements the solution in my last post and learned, that illustrator does stuff like this:

<?xml version="1.0" encoding="utf-8"?>
<!-- Generator: Adobe Illustrator 21.0.2, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->
<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
     viewBox="0 0 99 99" style="enable-background:new 0 0 99 99;" xml:space="preserve">

The line break inside the svg tag killed my initial regex, so I updated my last reply to detect line breaks in svg tags as well.

[Finesse]

@Neunerlei Your regular expression is not ready for negative values, e.g. viewBox="-243 144 13 13". Also it's not ready for multiple spaces. I recommend to use the following regular expression:

if(!parseSvg){
    // Check if we can read the viewbox to extract the height
    const viewbox = /<svg\s[^>]*viewBox\s*=\s*["']?\s*[\d.\-]+\s+[\d.\-]+\s+([\d.\-]+)\s+([\d.\-]+)\s*["']?/im.exec(source);
        // viewBox[1] - icon width, viewBox[2] - icon height; viewBox = null if there is no viewBox
        const height = viewbox && viewbox[2];
        if(!height) // Fail here...
}

@jantimon Hope to see this fixed soon. I can make a pull request if you agree with the viewbox solution.

[Neunerlei]

@Finesse Thanks for the hint and suggestion. I updated it both in the gist and my reply.

[matheusferraresi]

Do we really need all that workaround? Can't we just have an option like: 'checkSvgHeight = false' and the script jumps that checking part?

I can create a PR with that option added as I'm using it locally for my projects if you guys want.

[jantimon]

Version 4.1.0 drops the check for the svg height