feat(svg-sprite): Adds build sprite script with tests and docs by ivanquirino · Pull Request #319 · feathericons/feather

ivanquirino · 2018-02-19T16:02:58Z

Adds a build script for svg sprite. SVG attributes must be passed on the inline svg tag or through CSS class, as described in the docs for the sprite feature.

colebemis · 2018-02-19T17:43:22Z

This looks great. I'll do a thorough review soon 👍

colebemis

Awesome job! I just have a few comments.

colebemis · 2018-02-20T05:45:38Z

README.md


 All elements that have a `data-feather` attribute will be replaced with SVG markup corresponding to their `data-feather` attribute value. See the [API Reference](#api-reference) for more information about `feather.replace()`.

+#### 5. SVG Sprite


I feel like this heading should be on the same level as "Client-side" and "Node." The idea was that each subheading of "Usage" is a different way to use Feather. I'm planning to add sections like "React" and "Sketch" soon.

I'm already using the sprite with React, but I was thinking that using the icons.json file to render a React Component is even better.

Yeah, totally. I was just suggesting that the SVG Sprite section should be on the same hierarchical level as Client-side and Node.

colebemis · 2018-02-20T05:46:25Z

README.md

+}
+```
+```html
+<svg class="feather-default feather feather-[iconName]">


I think we could just use the .feather class to style the SVGs in this example.

colebemis · 2018-02-20T05:47:58Z

bin/build-sprite-function.js

+/**
+ * Renders a SVG symbol tag
+ * @param {*} name The name of the icon
+ * @param {*} contents The contents of the icon


Could you replace * with the actual type? In this case I think both of them should be string. Also it looks like this is missing an @returns statement

colebemis · 2018-02-20T05:49:49Z

bin/build-sprite.js

+import fs from 'fs';
+import path from 'path';
+import icons from '../dist/icons.json';
+import buildSprite from './build-sprite-function';


I'm not in love with calling this file build-sprite-function. I can't think of a better title right now though. I'll have to think about this one.

Maybe buildSpriteString and build-sprite-string.js since this function returns a string? I think I like that better than build-sprite-function.js.

ivanquirino · 2018-02-20T19:05:09Z

Good observations! I'll work on them and send another PR as soon as possible.

@colebemis

fixes requested by @colebemis

colebemis · 2018-02-20T19:25:24Z

bin/build-sprite-string.js

+
+const svgEndTag = ' </defs>\n</svg>';
+
+export default function buildSprite(icons) {


Since this file is called build-sprite-string.js, I think it makes sense to call this function buildSpriteString.

yeah, sorry!

colebemis · 2018-02-20T19:25:55Z

bin/build-sprite.js

+import fs from 'fs';
+import path from 'path';
+import icons from '../dist/icons.json';
+import buildSprite from './build-sprite-string';


Same thing here https://github.com/feathericons/feather/pull/319/files#r169431336

fixes the buildSpriteString function name

JSDoc for the buildSpriteString function

colebemis · 2018-02-20T19:31:51Z

README.md


 See the [API Reference](#api-reference) for more information about the available properties and methods of the `feather` object.

+### SVG Sprite


Awesome, can you add this to the table of contents?

yes, sure! I forgot that, I'm not used to contribute to such well made projects.

colebemis · 2018-02-20T19:32:36Z

README.md

+  stroke-width: 2; 
+  stroke-linecap: round; 
+  stroke-linejoin: round; 
+  fill: none


[Minor] Oops, we're missing a semicolon here.

colebemis · 2018-02-20T19:35:12Z

bin/__tests__/build-sprite.test.js

@@ -0,0 +1,12 @@
+/* eslint-env jest */
+import buildSpriteString from '../build-sprite-string';


Super minor, but this test should probably be called build-sprite-string.test.js because that's actually the function we're testing here.

* fixes add SVG sprite to the table of contents * fixes incorrect CSS example * renames the test correctly

colebemis · 2018-02-20T19:42:54Z

bin/build-sprite-string.js

+ * @param {object} icons the icons object
+ * @returns {string} the rendered string with SVG symbols
+ */
+export default function buildSpriteString(icons) {


I generally like to define the main function (i.e. buildSpriteString) at the top of the file and the helper functions (i.e. toSvgSymbol) after the main function. I find that it makes files a lot more readable. Using that style this page would be structured something like this:

imports... constants... function buildSpriteString (...) { ... } function toSvgSymbol(...) { ...} export default buildSpriteString

Here's a good example of this: https://github.com/feathericons/feather/blob/master/bin/build-icons-object.js

fix snapshot file name

file code styling

colebemis · 2018-02-20T20:53:18Z

bin/build-sprite-string.js

+function buildSpriteString(icons) {
+  const symbols = Object.keys(icons)
+    .map(icon => toSvgSymbol(icon, icons[icon]))
+    .reduce((spriteString, symbolString) => spriteString + symbolString, '');


Could we just use .join('') here instead?

colebemis · 2018-02-20T20:55:20Z

bin/build-sprite-string.js

+ * @returns {string} the rendered SVG symbol
+ */
+function toSvgSymbol(name, contents) {
+  return `    <symbol id="${name}" viewBox="${defaultAttrs.viewBox}">


Is there a reason we need to have add indentation to the strings? It might help with performance if we omit unneeded whitespace.

It was just for reading the svg. But since this is a build artifact, you are correct. I'm out now but I'm gonna fix this too.

colebemis

Awesome job! Thanks for being so responsive to my comments. Just give me the 👍 and I'll release this.

ivanquirino · 2018-02-21T00:14:48Z

@colebemis 👍 It's all ours now! 😄
Thanks for creating such great icons and a library that makes it easy to manipulate them. I'm thinking on coding a React component that uses icons.json directly. I think is the best way to make a React component for Feather. You may see another PR soon.

colebemis · 2018-02-21T00:29:45Z

Awesome, if you end up working on a React PR, here are my thoughts:

I'd do something like this: Making SVG icon libraries for React apps

I'd like to have a separate react-feather repo that uses the feather-icons npm package. This will allow us to use the icons directory to build the React components. With this approach, icons be used like this:

import Icon from 'react-feather'

<Icon name="circle" />

or

import IconCircle from 'react-feather/circle'

<IconCircle />

Since there is already a react-feather repo, it probably makes more sense to update that repo to use the approach outlined in the article above. I'd like to transfer that repo into the feathericons organization if @carmelopullara is on board.

carmelopullara · 2018-02-21T21:02:23Z

I agree with you, @colebemis. Since react-feather has become pretty popular on npm, I think it makes sense to update the repo and transfer it into the organization. I can start working on it this weekend.

feat(svg-sprite): Adds build sprite script with tests and docs

5996edc

ivanquirino mentioned this pull request Feb 19, 2018

SVG sprite #204

Closed