-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add prefixIds plugin #700
Add prefixIds plugin #700
Conversation
Add dom-walker file. Add tests. Add dependencies to package file.
Fix skipping in loop.
Add missing 'href' to condition.
Adjust test to test file names. Add identifier name escaping.
…ring). Improve prefix function name. Improve comments.
@@ -20,7 +20,7 @@ var SVGO = module.exports = function(config) { | |||
this.config = CONFIG(config); | |||
}; | |||
|
|||
SVGO.prototype.optimize = function(svgstr) { | |||
SVGO.prototype.optimize = function(svgstr, info) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why you don't pass that info
data in the config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The svgo architecture handles the config globally, it is harder to add per-plugin extra config than adding an extra argument to each plugin. Also this extra information is not config but rather environment/state instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, things like floatPrecision too, could be global instead of spread like now https://github.com/svg/svgo/blob/master/lib/svgo/config.js#L36 I'll see if it could be done with your way.
But if you want to do less changes, you could do it like ^, every plugin that needs a global config, defines it in the config, and it gets spread
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the file path must be somehow injected into the config object for each file (or set to empty/false/null for a string input): https://github.com/strarsis/svgo/blob/67e5a3ef10e349bded15bc9f508596252c2884be/lib/svgo/coa.js#L437
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea sorry again to be annoying, after that I stop:), but I think the best is to have an optional prefix
property at the root of the config and pass it to any plugin that needs it like https://github.com/svg/svgo/blob/master/lib/svgo/config.js#L34, it's more consistent with current code. The other option would be to refactor more and do var svgo = require('svgo'); svgo.optimize(svgStr, options);
passing whole config at each call, possibly in the form {common: {floatPrecision:.., prefix: '..'}, plugins: [...]}
so each plugin receive common and its plugin specific options. This plugin is interesting, even if it doesn't really minify, I use svgo to make png/jpg previews of them, and I do that in iframes to avoid those conflicts, so your way could make it work too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@caub: Yes, a separate prefix config option is supported now (see https://github.com/svg/svgo/pull/700/files#diff-623dc50b643d62febf2af73ea1856b51R120).
plugins/prefixIds.js
Outdated
|
||
|
||
var path = require('path'), | ||
csstree = require('css-tree'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those 3 deps are really not necessary
just put the regex you need, and for tree walking it's just
function walk(node, cb) {
cb(node);
if (node.content) node.content.forEach(walk)
}
but make sure you can't use 'perItem' mode first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I use perItem now.
Remove dom-walker.
Edit: Referenceable attribute check added. Currently all attribute values are checked for url(...) / ID references. |
plugins/prefixIds.js
Outdated
|
||
exports.type = 'perItem'; | ||
|
||
exports.active = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also I wouldn"t enable it by default
Improve code. Improve tests.
Clean up log.
FWIW, I just noticed that Illustrator CC offers 'Unique' for 'Object IDs' in SVG export options. |
Jeah we also found this option but we also noticed that illustrator export is not suitable in every case. |
# Conflicts: # lib/svgo/coa.js # package.json
@GreLI: Conflicts fixed, ready to merge! |
👍 Yes! |
Awaiting for the version bump! |
Very good! |
in order to support prefixIds svgo rule. @see: svg/svgo#700
I want to cancel the class prefix in the svg file, What should I do? |
@TongDaDa: You can disable plugins you don't want to use. |
in order to support prefixIds svgo rule. @see: svg/svgo#700
in order to support prefixIds svgo rule. @see: svg/svgo#700
This PR adds the prefixIds plugin which prefixes all identifiers (IDs, class names) and
also updates the url(...) references.
Additionally support for extra information is added that can be optionally used by the plugins,
currently containing the path to the input SVG file or that mode is text (when text input is used).
Related issues: #494 #595 #623 #673 #674
Similar approach would be inlining all styles (see the inlineStyles plugin) -
though this is not possible with elements that are
<used/>
(see #651).This prefixing approach could be viewed as a "polyfill" (partially) for scoped styles.