A few straight refactors in badge-renderers #5716
Conversation
|
| if (minify) { | ||
| return stripXmlWhitespace(badge) | ||
| } |
There was a problem hiding this comment.
Could this change potentially have any behavioral impacts? Or was minify always truthy for all badge styles previously?
There was a problem hiding this comment.
No, I don't see how it could. badge-renderers.js is only used in make-badge.js, and in the only spot where the function is invoked it gets minify: true.
There was a problem hiding this comment.
If I ever have to read the SVG outputs by eye, I usually flip minify off. How do you normally deal with that?
There was also a point where we were using it to make the snapshot tests more readable #4459 (comment) but then we switched to minified snapshots later, although I notice you've taken another approach to that elsewhere which should be less sensitive to whitespace changes, so hopefully best of both worlds there.. 👍
There was a problem hiding this comment.
Ah, interesting. My workflow for that hasn't been great. Usually I save out the svg's from the browser and then open them in VS Code and format them.
I'd be happy to add an env var that turns off the minification, if you think that would be useful.
The formatted snapshots seem to be super nice, though! I imagine that may end up being sufficient.
There was a problem hiding this comment.
Nah. Thinking about it,
- return stripXmlWhitespace(
+ return (isn't exactly much more effort than
- minify: true,
+ minify: false,on the odd occasion we want that - Its not that common.
There was a problem hiding this comment.
I didn't make it down to make-badge.js last night, would've been good to do so beforehand as I would've seen minify was always true 😆
These are aimed at improving readability and a bit of DRY.