Addition fix to previous fix for #1093 #1140
Conversation
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!
| @@ -110,38 +108,38 @@ export const setupHandlers = grnState => { | |||
| } | |||
| } | |||
| }; | |||
| visit(svg); | |||
| visit(node); | |||
| return tree; | |||
| }; | |||
|
|
|||
| 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); |
| if (value !== emptySvgDeclarationComputed.getPropertyValue(key)) { | ||
| // 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
|
|
||
| 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.