Refactor: svg group fix and blur #36
Conversation
| "files": [ | ||
| "src", | ||
| "vendor" | ||
| ], |
| @@ -1,4 +1,5 @@ | |||
| #! /usr/bin/env node | |||
| #!/usr/bin/env node | |||
|
|
|||
There was a problem hiding this comment.
Recommendation from eslint-plugin-node :)
| } | ||
|
|
||
| // Remove x and y attributes since they default to 0 | ||
| // @todo test in rare browsers |
There was a problem hiding this comment.
This is because there is no need to have a background rectangle which contains the exact width of the svg. The engine can calculate this on its own. So a 0 0 100% 100% sizing should be totally fine.
Codecov Report
@@ Coverage Diff @@
## next #36 +/- ##
=========================================
+ Coverage 93.4% 94.59% +1.18%
=========================================
Files 6 6
Lines 91 111 +20
Branches 11 13 +2
=========================================
+ Hits 85 105 +20
Misses 5 5
Partials 1 1
Continue to review full report at Codecov.
|
|
|
||
| exports[`replaceSVGAttrs svg with group, config with dimensions only 1`] = `"<svg xmlns=\\"http://www.w3.org/2000/svg\\" viewBox=\\"0 0 1024 640\\"><filter id=\\"b\\"><feGaussianBlur stdDeviation=\\"12\\" /></filter><path fill=\\"#bada55\\" d=\\"M0 0h1024v640H0z\\"/><g filter=\\"url(#b)\\"><path fill=\\"#C0FFEE\\" d=\\"M51.5 17.5l4 18 15 1z\\"/></g></svg>"`; | ||
| exports[`applies blur filter svg with group and blur 1`] = `"<svg viewBox=\\"0 0 1024 768\\"><filter id=\\"b\\"><feGaussianBlur stdDeviation=\\"5\\"/></filter><rect fill=\\"#bada55\\"/><g filter=\\" url(#b)\\"><g><path fill=\\"#C0FFEE\\" d=\\"M51.5 17.5l4 18 15 1z\\"/></g></g></svg>"`; |
There was a problem hiding this comment.
I wonder if the xmlns attribute should be required by us. AFAIK svgo adds it, but as we do already ensure there is a viewbox we could do more optimizations to ensure the svg will render properly in all browsers ❓
|
I have to admit that the test names are not that ideal, I might change it before merging 🙈 |
|
@efegurkan While this is still slightly in progress (mostly the tests), I'd love to get some feedback :) |
|
This seems good. For the tests, since you are changing the API a lot, I will suggest not to care too much on Unit tests until you feel good about the plugin. When we did first implementation with @technopagan I made the code as much as "pipeline-ish" sense. In theory all of this parts can be moved to separate modules as you did in this PR. And most of them can be written side-effect free. 🎉 You may want to move svg templates to folder like fixtures maybe to make the tests more readable for future cases. And I will suggest separating the default plugins to three part as:
Feel free to name them as you like ofc :) Very nice PR 👍 |
|
Great and thanks for your nice words. So, I will do clean up and proper naming later on, I like that very much. Merged it, next step is async API 🥂 |
This is my first attempt to change the inner code logic of sqip on the road to a plugin based system (See #30)
It does the following: