search

nfroidure / svgicons2svgfont

Concatenate SVG icons and ouput an SVG font
http://nfroidure.github.io/svgiconfont/
MIT License
340 stars 71 forks source link

Add color property to glyph object #48

Closed TylerYang closed 8 years ago

TylerYang commented 8 years ago

Map attribute 'fill' to color property

nfroidure commented 8 years ago

Unfortunately the fill property content is a <paint> value http://www.w3.org/TR/SVG/painting.html#SpecifyingPaint

It means it is a bit complexer than just specifying a color. You should at least test if the attribute value is a color and then fill it only if it match. You also could pick parent fill value if the inherit value is found (see this function that does a similar job https://github.com/nfroidure/svgicons2svgfont/blob/a28c981bfff2dc49f999a358f52728beea09bae0/src/index.js#L43 )

TylerYang commented 8 years ago

That make sense, I will PR a new one later.

Here is my question, What if there are more than one fill attribute in svg element and their value are different?

For example, 

<svg>
    <g> 
        <path fill="#F00"  d="M4.." />
    </g>
    <path fill="#CCC" />
</svg>

In this case, I will simply set the last fill attribute(Only if it is a valid value) to glyph.

TylerYang commented 8 years ago

Hi @nfroidure , do you accept with this PR? Because this is really important for my project. Any comments are welcome. Thx.

nfroidure commented 8 years ago

Just looked at your PR but it comes with no test at all. Can't merge without at least one test.

TylerYang commented 8 years ago

My bad, I will submit the test case tomorrow. Thx...

TylerYang commented 8 years ago

@nfroidure Hi, just added some test cases. Looking forward to your advise. Thx.

nfroidure commented 8 years ago

@TylerYang sorry for the elapsed time, couldn't review your work yet. I'll look at it this week.

TylerYang commented 8 years ago

@nfroidure It's OK, I will be waiting.

TylerYang commented 8 years ago

Can you review this PR also? https://github.com/nfroidure/gulp-svgicons2svgfont/pull/31 It failed to pass the tests because in preview svgicons2svgfont module, it didn't have the color property.

nfroidure commented 8 years ago

You can easily make it pass by using your repo's url. I'll try to review it asap anyway.

TylerYang commented 8 years ago

@nfroidure I would like to do it so, but seems in China, I cannot access to this url at this moment. All I can get is ERR:TIMEOUT from their CDN.

Anyway, thx for your help. This lib did improve the dev&prod experience a lot.