"lazy load" CSS #78
"lazy load" CSS #78
Conversation
| + '<img id="deepcat-smallhint-hide" title="' + mw.msg( 'deepcat-smallhint-close' ) + '" src="https://upload.wikimedia.org/wikipedia/commons/4/44/Curation_bar_icon_close.png">' | ||
| + mw.msg( 'deepcat-hintbox-small' ) | ||
| + '</div>'; | ||
| $( '#searchform' ).after( smallHintBox ); | ||
| $( '#deepcat-smallhint-hide' ).on( 'click', hideSmallHint ); | ||
| } | ||
|
|
||
| function loadHintCodeIfNeeded() { |
There was a problem hiding this comment.
This is not a good function name. It seems it does more than "loading code". See the add… calls below.
| @@ -1,19 +1,8 @@ | |||
| /** | |||
| * DeepCat Gadget for MediaWiki | |||
| * @licence GNU GPL v2+ | |||
| * @author Christoph Fischer < christoph.fischer@wikimedia.de > | |||
| * @author Christoph Jauera < christoph.jauera@wikimedia.de > | |||
There was a problem hiding this comment.
This name change should be a separate patch, but this is not critical.
👍 overall, looks good.
There was a problem hiding this comment.
True. Will do it in an other patch :-)
WMDE-Fisch
force-pushed
the
lazy-load-css
branch
from
0a9e5f2
to
d06d563
Compare
|
/me pokes @manicki or @jakob-WMDE to have a 2nd look on this PR |
There was a problem hiding this comment.
Had a very quick look. Mostly old man nagging about function names. Please consider if my comment on CSS paths is relevant here, though.
I'll have another look on the patch today and will then verify it is actually working. Generally looking fine for me, we might be able to have this merged today!
| hintCodeLoaded = true; | ||
| } | ||
|
|
||
| function loadThrobberCodeIfNeeded() { |
There was a problem hiding this comment.
code seems a bit mysterious, then I check what it does, and it load styles. Maybe loadThrobberStyles or loadThrobberResources? I am not sure if I'd put the "IfNeeded" part to the method names but it might be just matter of taste, I am not saying you should change it one way or another.
There was a problem hiding this comment.
good point will change that
| + '<img id="deepcat-smallhint-hide" title="' + mw.msg( 'deepcat-smallhint-close' ) + '" src="https://upload.wikimedia.org/wikipedia/commons/4/44/Curation_bar_icon_close.png">' | ||
| + mw.msg( 'deepcat-hintbox-small' ) | ||
| + '</div>'; | ||
| $( '#searchform' ).after( smallHintBox ); | ||
| $( '#deepcat-smallhint-hide' ).on( 'click', hideSmallHint ); | ||
| } | ||
|
|
||
| function initializeHintCodeIfNeeded() { |
There was a problem hiding this comment.
On first glimpse at the code I am really confused what "hint code" is.
Looking at the method what it does, it loads css styles and adds some HTML elements. That's OK but which of those two is the actual "code"? Or is the "hint code" something different hidden somewhere deeper?
Wouldn't initializeHintBoxes(IfNeeded)? sound more clear in this case?
There was a problem hiding this comment.
good point will change that as well :-)
| if( hintCodeLoaded ) { | ||
| return; | ||
| } | ||
| mw.loader.load( cssPath + 'DeepCat.hintbox.css&action=raw&ctype=text/css', 'text/css' ); |
There was a problem hiding this comment.
Don't we want to put those CSS paths in some config-like variable (on top of the file or so), so it is easier to customize and paths are not hidden somewhere deep in private methods?
There was a problem hiding this comment.
atm the path is in a config-like var at the top of the file :-)
There was a problem hiding this comment.
Bah, yeah. What I probably meant was having full paths (i.e. including filename) as config variables. But this indeed might not be necessary. Unless we believe it is likely to want to have those CSS files named different when gadget is copied over to some other place or whatever other evil things happens. But shortly: right, it is fine as it is now!
moved CSS for hint-boxes and throbbers to separate files corrosponding CSS will only be loaded if really needed
WMDE-Fisch
force-pushed
the
lazy-load-css
branch
from
d06d563
to
6dc4edf
Compare
|
Looks good to me now! |
moved CSS for hint-boxes and throbbers to separate files
corresponding CSS will only be loaded if really needed
see https://phabricator.wikimedia.org/T144168