Conversation
dondi
left a comment
There was a problem hiding this comment.
Good find! For this one, I realize you are revising pre-existing code, but it looks like the pre-existing code itself could have used some refactors. Please see if we can switch from a concatenated string to Object.assign when building up the new inline style for the node(s)—thanks!
| }; | ||
|
|
||
| const explicitlySetStyle = element => { | ||
| const cSSStyleDeclarationComputed = window.getComputedStyle(element); |
There was a problem hiding this comment.
I realize this was pre-existing code, but since you’re already changing this, let’s fix the capitalization issue: the CSS here should actually be all lowercase, if we are to follow the convention:
| const cSSStyleDeclarationComputed = window.getComputedStyle(element); | |
| const cssStyleDeclarationComputed = window.getComputedStyle(element); |
| // Don't set computed style of width and height. Makes SVG elmements disappear. | ||
| if ((key !== "height") && (key !== "width")) { | ||
| computedStyleStr += key + ":" + value + ";"; | ||
| computedStyleStr += `${key}:${value};`; |
There was a problem hiding this comment.
Great change! Yes, in the age of template strings I generally change any concatenations + into template/formatted strings 💯
| if (element.classList.contains("weight")) { | ||
| computedStyleStr += "visibility: hidden;"; | ||
| } | ||
|
|
||
| if (computedStyleStr) { | ||
| element.setAttribute("style", computedStyleStr); | ||
| } |
There was a problem hiding this comment.
The observation here is that we are building a string in order to set what is really an object—style attribute names + their values. In the end, I think this whole block will be more robust if we build the desired style as an object instead of a string. Then, Object.assign can be used to set the final style once the object is built. Here’s a link to show how this would work: https://www.bram.us/2017/01/16/using-object-assign-to-quickly-set-multiple-inline-styles/
This does change this block of code quite a bit, but I think it is a good change in the right direction—what do you think?
There was a problem hiding this comment.
I think Object.assign is better!
| while (i--) { | ||
| explicitlySetStyle(allElements[i]); | ||
| } | ||
| allElements.forEach(explicitlySetStyle); |
There was a problem hiding this comment.
Another great refactor, using the function-as-iterator trick from a prior PR 🤩 Also nice to see 4 lines become just one, right? 😁
…le to reexisting nodes
dondi
left a comment
There was a problem hiding this comment.
That Object.assign change works really well I think! But I think you missed one more capitalization change?
For things like that, VS Code’s Rename command really helps—I can show it to you if you aren’t familiar with it yet
|
|
||
| for (let i = 0; i < cssStyleDeclarationComputed.length; i++) { | ||
| const key = cssStyleDeclarationComputed[i]; | ||
| const value = cSSStyleDeclarationComputed.getPropertyValue(key); |
There was a problem hiding this comment.
Whoops, shouldn’t this be edited also?
| const value = cSSStyleDeclarationComputed.getPropertyValue(key); | |
| const value = cssStyleDeclarationComputed.getPropertyValue(key); |
There was a problem hiding this comment.
I didn't catch this one. Usually, I would use Ctrl + D to select all words, but I think I was fixing it manually. But I would love to learn about VS Code's Rename command!
|
I checked this in beta, and it works as expected! |
When exporting the svg, using cloneNode making the image to be all black. This is because style didn't pass correctly. This makes sure the style is correct.