GH-1975 Theme Framework, Leaf & Palm Themes (FINAL) #525
Conversation
benstrumeyer
commented
Apr 14, 2020
|
…z feature buttons
|
A few cleanup & modernization comments on Also, I remember this being discussed in the comments for an earlier version of this PR, but, ....are we sure we want to bundle the theme PNGs with the extension? That's over a megabyte of data that the overwhelming majority of our users will never use (and should not even have access to). And what happens if we add 3 more themes later this year? |
| // Other kinds of loops are not supported equally across browsers | ||
| let themeStyle = null; | ||
| for (let i = 0; i < styleList.length; i++) { | ||
| const style = styleList[i]; | ||
| if (style.title.startsWith(styleTitlePrefix)) { | ||
| doc.head.removeChild(style); | ||
| themeStyle = style; | ||
| } | ||
| } |
wlycdgr
Apr 14, 2020
Member
We can remove this comment and replace with the tidier forEach, which is now supported in all our supported browser versions
We can remove this comment and replace with the tidier forEach, which is now supported in all our supported browser versions
benstrumeyer
Apr 15, 2020
Author
Contributor
Good to know, changed to a forEach loop
Good to know, changed to a forEach loop
benstrumeyer
Apr 15, 2020
•
Author
Contributor
I stand corrected, we're unable to use a regular forEach loop because getElementsByTagName returns an HTML collection. I've changed it to Array,prototype.forEach.call(styleList, (style) => {...}) as suggested by the following stackoverflow thread unless you think keeping a basic for loop is cleaner. Thoughts?
I stand corrected, we're unable to use a regular forEach loop because getElementsByTagName returns an HTML collection. I've changed it to Array,prototype.forEach.call(styleList, (style) => {...}) as suggested by the following stackoverflow thread unless you think keeping a basic for loop is cleaner. Thoughts?
wlycdgr
Apr 15, 2020
Member
Ah dang, right, sorry. We can use Array.from(styleList).forEach
Ah dang, right, sorry. We can use Array.from(styleList).forEach
| for (let i = 0; i < styleList.length; i++) { | ||
| const style = styleList[i]; | ||
| if (style.title.startsWith(styleTitlePrefix)) { | ||
| doc.head.removeChild(style); | ||
| } | ||
| } |
wlycdgr
Apr 14, 2020
Member
Here too we can now use forEach
Here too we can now use forEach
benstrumeyer
Apr 15, 2020
Author
Contributor
Changed to a forEach loop
Changed to a forEach loop
benstrumeyer
Apr 15, 2020
Author
Contributor
Same issue as above
Same issue as above
|
@wlycdgr I suggested that we add the background PNG for one of the themes to the extension in a previous PR, but when you put it that way, especially in regards to future theme releases, I agree that it might be better not to. @benstrumeyer Would it be a huge lift to switch back to the way you had it when you were passing the PNG with the themed Sass file? |
|
LGTM |
