Closed systemtwo closed 11 years ago
Lots of work to do tonight, so I won't get a chance to properly merge/comment on this until tomorrow at the earliest.
However, a few quick comments:
script.onload
. Might be overkill, this one's up to you.value:property
is sometimes value: property
in css. I prefer the latter, but it doesn't really matter as long as it's consistent.So it turns out the IE loading bug is because dynamically loading scripts does not delay the onload call. So it may be best if all of the include scripts are pulled out to regular script tags inside the HTML. This will result in fast loading, should remove the IE bug and will make the progress divisions as clear as possible.
This would also make the progress more accurate, preventing the progress from stopping or slowing due to dynamically loaded scripts and instead having the progress bar actually represent our best guess of the progress left.
Script tags or dynamically loaded makes no difference really, but yes I agree they should probably all be in one list somewhere. In buildblast, we actually generate this list using a simple python script, which has the advantage that you can generate both dynamic loader code and automatic minification code automatically. On May 24, 2013 12:00 PM, "yeerkkiller1" notifications@github.com wrote:
So it turns out the IE loading bug is because dynamically loading scripts does not delay the onload call. So it may be best if all of the include scripts are pulled out to regular script tags inside the HTML. This will result in fast loading, should remove the IE bug and will make the progress divisions as clear as possible.
This would also make the progress more accurate, preventing the progress from stopping or slowing due to dynamically loaded scripts and instead having the progress bar actually represent our best guess of the progress left.
— Reply to this email directly or view it on GitHubhttps://github.com/gitdefence/game-off-2012/pull/124#issuecomment-18413901 .
It also means all scripts in the scripts folder are included automatically (after the script is run, anyway), which is nice to avoid having dead scripts.
This is how we do includes in buildblast:
https://github.com/crazy2be/buildblast/blob/master/client/js/genincludes.py
https://github.com/crazy2be/buildblast/blob/master/client/js/includes.js
So, the ideal solution would be something like what we do in buildblast (see above links). This makes minification easier, ensures all existing files are actually used, and is generally just nicer.
However, that does take a bit more work, so I would accept a pull request (And i suspect @yeerkkiller1 would as well, although that's up to him) which simply hard-codes all of the script paths, getting rid of the multiple include.js files. This would allow us to better support IE (current onload behaviour is a flaky hack), while being a little bit simpler than having python generate it. It also gives us a easy transition to having python generate the list and minified scripts in the future.
Other things that would be nice:
LGTM.
Addressing the first part of Issue #120