add option to removeDimensions #58
Conversation
Codecov Report
@@ Coverage Diff @@
## master #58 +/- ##
==========================================
+ Coverage 46.27% 46.74% +0.46%
==========================================
Files 19 20 +1
Lines 255 261 +6
Branches 52 54 +2
==========================================
+ Hits 118 122 +4
- Misses 116 117 +1
- Partials 21 22 +1
Continue to review full report at Codecov.
|
|
the https://github.com/svg/svgo/blob/master/plugins/removeDimensions.js This is a good article explaining those properties. https://www.sarasoueidan.com/blog/svg-coordinate-systems/ And also you don't need the |
There was a problem hiding this comment.
@lifeiscontent thanks!
Can you please speak with @wuweiweiwu to decide which PR is the most relevant. For me you are the first so you are legit, but if you prefer to let @wuweiweiwu it is your choice.
- Documented option is still missing in README (you updated only CLI)
src/configToOptions.js
Outdated
| @@ -57,6 +58,7 @@ function configToOptions(config = {}) { | |||
| const svgoConfig = { plugins } | |||
| if (!config.title || config.icon) plugins.push({ removeTitle: {} }) | |||
| else if (config.title) plugins.push({ removeTitle: false }) | |||
| if (!config.dimensions) plugins.push({ removeDimensions: true }) | |||
There was a problem hiding this comment.
As @wuweiweiwu this seems to not be sufficient.
|
@neoziro updating now, please stand by. 👍 |
|
@neoziro do you mind that I'm sorting the options/docs? If not, I'll do that, but I don't think I'll be able to do it in 1 commit. I've already done it, but I can revert if necessary. |
|
@neoziro also added docs for some undocumented stuff, please review. |
|
@neoziro @lifeiscontent I think you should have it! :) |
src/configToOptions.js
Outdated
| config.replaceAttrValues.forEach(([oldValue, newValue]) => { | ||
| plugins.push(replaceAttrValue(oldValue, newValue)) | ||
| }) | ||
| if (!config.dimensions) { | ||
| plugins.push(...['width', 'height'].map(stripAttribute)) |
There was a problem hiding this comment.
@neoziro addressed this in my PR but this will remove all height and width attributes. Regardless if its in an svg or not.
I think the best way is to actually implement a h2x plugin similar to emSize
Something like:
const removeDimensions = () => ({
visitor: {
JSXElement: {
enter(path) {
// Skip if not svg node
if (path.node.name !== 'svg') return
const otherAttributes = []
path.node.attributes.forEach(attr => {
if (attr.name === 'width' || attr.name === 'height'){
// NO OP
}
else otherAttributes.push(attr)
})
path.node.attributes = otherAttributes
},
},
},
})I've tested this and it works :)
|
Also I think its best practices to not change stuff outside the scope of the PR. For example. Your doc edits should probably be in another PR. cc: @neoziro |
|
@lifeiscontent your documentation reordering is OK for me in that PR, it is not optimal but it is OK. Thanks for taking care of it.
Thanks! |
|
@wuweiweiwu I agree with you about the scope, I kinda went down a small rabbit hole, but I figured since this project had given me a lot of benefit, I might as well knock out some cleanup work as well, and as you can see in my previous post to @neoziro I told him I'd happily pull it out. anyway, @neoziro I've updated the docs and reimplemented @wuweiweiwu's implementation for removeDimensions. |
|
@lifeiscontent thanks you!! |
I'd like to be able to specify width/height within props, this lets me do that.
@neoziro
I also took the liberty to sort the config options/defaults/docs. Let me know if you want me to pull that out.
I made them separate commits, so you don't get overloaded with a diff when reviewing.