Replaced MochiKit with jQuery
Review Request #212 - Created June 20, 2012 and submitted
Information | |
---|---|
Adrian Budau | |
infoarena | |
Reviewers | |
hackers | |
Replaced all Mochikit js with Jquery. This was just a replace so hacks still remain. Testing not yet done, still it's jquery and that should guarantee us cross-browser compatibility.
Review request changed
Change Summary:
Just some things I did not see at that hour which would've come out at a review anyway.
Diff: |
Revision 2 (+145 -8020) |
---|
This is a good start, well done. A few things: - Is there any reason you were using $(document).one('ready') in some places and $(window).one('load') in others? - Did you check that the inlined javascript we have in some views still works? For example, in www/views/task_tags, i'm seeing $$("#add_category > a")[0], which I'm fairly sure is from the MochiKit era. - This is as a TODO: throughout the code we are using element ids where we should be using classes. For example, there is no reason we should have a single #remotebox on a page. We should be able to have multiple .remotebox elements on a page, each pointing to their individual url. There's still a long way to go before we have decent JS, but this diff is a step in the right direction. :)
-
trunk/common/newsletter_template.php (Diff revision 2) -
See comment in www/views/header.php
-
trunk/www/static/js/default.js (Diff revision 2) -
You can just $(document).ready(Page_Init); Probably better to change all of them to this.
-
trunk/www/static/js/newsletter.js (Diff revision 2) -
$(document).ready(...) should work as well.
-
trunk/www/static/js/parameditor.js (Diff revision 2) -
Why don't you just use .show() ?
-
trunk/www/views/header.php (Diff revision 2) -
Specify a jQuery version explicitly. You can load jquery from Google's datacenters, which apparently results in increased parallelism: http://encosia.com/3-reasons-why-you-should-let-google-host-jquery-for-you/ I would suggest something like if (IA_DEVELOPMENT_MODE) { ... src="js/jquery-1.7.2.js" ... } else { ... src="https://ajax.googleapis.com/ajax/libs/jquery/1.7.2/jquery.min.js" ... } Notice that when not in development mode, the minified version is sent to the user, resulting in better loading times.
-
trunk/www/views/header.php (Diff revision 2) -
Afaik, this conflicts with $(document).ready() I don't think we use it anywhere in the code, so please just remove it.
Review request changed
Change Summary:
Hopefully fixed the problems. Sorry for the 2 big files(Mochikit and jquery). Tested every script in www/static/js which was made by infoarena and they work as expected Added task_tags.js for the adding tags page.
Diff: |
Revision 3 (+9611 -8059) |
---|
Awesome. :D Hopefully everything works fine.