Scoped themes to apply only to the editor (#editor-holder) #8447
Conversation
| @@ -156,7 +156,7 @@ define(function (require, exports, module) { | |||
| filename: fixPath(theme.file._path) | |||
| }); | |||
|
|
|||
| parser.parse("." + theme.className + "{" + content + "}", function (err, tree) { | |||
| parser.parse("." + theme.className + " #editor-holder {" + content + "}", function (err, tree) { | |||
There was a problem hiding this comment.
Just to clear up my own confusion... :-) Why is the theme.className prefix needed? I think in another thread you mentioned you're deleting the <style> tag when a theme is turned off... so is there any need to have a class placed on the root of the DOM if that class will always be present when the theme's rules are in memory?
There was a problem hiding this comment.
Actual, you are right. We don't need that. Cleaning it up...
There was a problem hiding this comment.
To answer the question though. There is no need for the root class because I remove the style nodes from the DOM. Cant get anything passed you, love it :D
There was a problem hiding this comment.
Haha, sorry! Maybe I should try out as a soccer goalie ;-)
|
It was a bit more involved than I thought to prevent flickering and properly handle unloading the style nodes... cc @dangoor |
|
Starting review... The tests are broken. |
|
|
||
| return $.when.apply(undefined, pendingThemes); | ||
| return $.when(pending); |
There was a problem hiding this comment.
pending is just a single promise now, right? So, you can just return it directly?
There was a problem hiding this comment.
Ah yes. I commited those changes last night but didn't push. Will do that in a minute
There was a problem hiding this comment.
Oh no what's there is good. There is a the check if the theme is not falsy before it reads the text. The $.when is just a shortcut to always return a promise
There was a problem hiding this comment.
Oh, I see... it's not necessarily a promise if the current theme is null or undefined.
|
Oh yes, there are some jsdoc changes that are needed as well because of the switch from "current themes" to "current theme". Review done. I'll look into making the needed changes or filing followups so that I can work based on this code.
|
| * @return {Array.<Theme>} collection of all available themes | ||
| */ | ||
| function getAllThemes() { | ||
| return _.map(loadedThemes, function (item) { | ||
| return item; | ||
| return loadedThemes.map(function (theme) { |
There was a problem hiding this comment.
ahh, this doesn't work because loadedThemes is an object and doesn't have map. _.map will take an object and iterate on its values.
|
I noticed something in the tests that I hadn't before: the async tests are missing the In the process of reformatting those tests a bit to fit how Jasmine works, I noticed that |
|
I've split off the |
|
Hey Kevin... Let me go over what you have reviewed... I was winging it from a docs appointment, but i need to go through it again. So, it's very apparent I missed several small little issues. Sorry man, I guess I was more tired than I thought :) |
|
No problem! It was just minor stuff. I've done the fixups locally already. |
|
Will be merging and pushing this momentarily. |
|
Sweet! Thanks dude! |
|
So, yeah I did end up removing all the handling for multiple themes. It seemed to have caused more confusion than anything else... Also, removing the loadDirectory stuff is fine. I will probably just make a gist somewhere and reference it in BoilerPlate. |
|
OK, I've removed |
#8415
#8420