Updated replace() to pass id from placeholder element #193
Conversation
colebemis
left a comment
There was a problem hiding this comment.
Awesome job! Just left a few minor comments.
src/replace.js
Outdated
| } | ||
|
|
||
| const elementClassAttr = element.getAttribute('class') || ''; | ||
| const elementId = element.getAttribute('id'); |
There was a problem hiding this comment.
I think this should probably be elementIdAttr to be consistent with elementClassAttr.
| combinedOptions.class = addDefaultClassNames(combinedOptions.class, key); | ||
|
|
||
| const attributes = optionsToAtrributes(combinedOptions); | ||
| const attributes = optionsToAttributes(combinedOptions); |
There was a problem hiding this comment.
Haha thanks for catching my typo!
|
|
||
| Object.keys(options).forEach(key => { | ||
| attributes.push(`${key}="${options[key]}"`); | ||
| if (options[key]) { |
There was a problem hiding this comment.
When would options[key] ever be falsey in this context? Object.keys(options) is generating an array of all the keys in the object and we're using forEach to iterate over all the keys. I'm not sure options[key] would ever be undefined.
There was a problem hiding this comment.
In this case it was the lack of a value for id where element.getAttribute('id') results in null. This doesn't cause any errors but ends up with an eventual id="null" attribute on the SVG element. The only other situation where I could see this being falsey is if someone was to pass a null value for an option when using replace or toSvg.
I could instead check if id has a value before passing it as an option to toSvg, let me know if that would be preferable here.
There was a problem hiding this comment.
Gotcha. I didn't think about that. I think this is fine!
There was a problem hiding this comment.
It may be a good idea to use TypeScript to catch these kinds of bugs 😄
Thanks for the awesome icons, I saw a chance to contribute with #186 and figured I would open up a PR. It does the following.
toSvg()options parameter.optionsToAttributes()a bit more nullsafe in caseidis undefined (or any other attributes that may be added), fixed minor typo.Closes #186