fixed change_list.js bug in IE and provided a fix to namespacing issues ... #1372

Merged
merged 19 commits into from Aug 14, 2012

Projects

None yet

7 participants

@andrewschoen
Contributor

Fixed the bug @piquadrat found in the change list with IE. Also, I think I've resolved our namespace issues with closures. Closures ftw!

@travisbot

This pull request passes (merged 79c4c6a into 46506b7).

@piquadrat piquadrat and 1 other commented on an outdated diff Aug 3, 2012
cms/static/cms/js/change_list.js
@@ -276,13 +276,26 @@
// the other click event on this element to fire
jtarget.addClass("loading");
var pageId = $(jtarget).attr("id").split("page_")[1];
+ // the following is added because IE is stupid
+ // ref: http://stackoverflow.com/questions/4557532/jquery-ajax-requests-failing-in-ie8-with-message-error-this-method-cannot-be-c
+ $.ajaxSetup({
@piquadrat
piquadrat Aug 3, 2012 Contributor

would it perhaps make sense to move this workaround into a code path that is available for all Ajax requests, e.g. the $.ajax wrapper at the top of this file, or even into cms.base.js?

@andrewschoen
andrewschoen Aug 3, 2012 Contributor

That does make more sense. Moved to cms.base.js

@piquadrat piquadrat and 1 other commented on an outdated diff Aug 3, 2012
cms/static/cms/js/plugins/cms.setup.js
-// insuring django namespace is available when using on admin
-var django = django || undefined;
+ // assigning correct jquery instance to jQuery variable
+ jQuery = (django) ? django.jQuery : window.jQuery || undefined;
@piquadrat
piquadrat Aug 3, 2012 Contributor

Sticking a var in front of this would take care of #1349

@andrewschoen
andrewschoen Aug 3, 2012 Contributor

I think the closure takes care of that, but I went ahead and added it for good measure. Won't hurt anyway.

@piquadrat
Contributor

I can confirm that this fixes both the inital load error (#1350) and the lazy menu. With the little var tweak mentioned above, it could also fix #1349.

Also, we use tabs instead of 4 spaces in everything frontend (js/css/html). Could you fix that in the patch?

Otherwise, awesome work debugging and fixing this!

@andrewschoen
Contributor

Ok, I've got all the feedback worked in.

Just curious, why tabs & spaces? I'm perfectly happy to change it to stay consistent with the project, but just seems kinda strange to do that.

@travisbot

This pull request passes (merged f67a791 into 46506b7).

@piquadrat
Contributor

@andrewschoen you'd have to ask @FinalAngel about the tabs convention :)

@ojii
Collaborator
ojii commented Aug 3, 2012

wait I thought we use tabs in JS, and 4 spaces in Python. That would make the most sense to me...

@andrewschoen
Contributor

@ojii, yeah that's how I did it. Tabs in js and spaces in Python.

Typically I'd 4 space all the things. But like I said, project consistency if important so it's whatever works.

@andrewschoen
Contributor

I did find quite a few references to files using jQuery instead of CMS.$ in plugins and such. Would it be worth it to find all those, wrap them in a closure and pass them CMS.$ ?

That being said, I didn't find anything else broken though either.

@travisbot

This pull request passes (merged d25ec25 into 46506b7).

@travisbot

This pull request passes (merged 887e45a into 46506b7).

@digi604
Member
digi604 commented Aug 13, 2012

is this ready?

@FinalAngel FinalAngel was assigned Aug 13, 2012
@andrewschoen
Contributor

@digi604 it should be. Things got a bit weird on the rebase, does this still merge clean?

I tested pretty well but I could probably use another set of eyes on the plugin changes. I"ll do another round of testing this afternoon as well.

@travisbot

This pull request passes (merged 0d74519 into c23c632).

@andrewschoen
Contributor

I just ran my tests again. Still working great for me.

@digi604 digi604 commented on an outdated diff Aug 13, 2012
...ins/text/templates/cms/plugins/widgets/wymeditor.html
+ {{ WYM_CLASSES }}
+ ],
+ editorStyles: [
+ {{ WYM_STYLES }}
+ ],
+ {% if WYM_STYLESHEET %}
+ stylesheet:
+ {{ WYM_STYLESHEET }}
+ ,
+ {% endif %}
+ postInit: function(wym) {
+ //wym.resizable({handles: "s", maxHeight: 600});
+ //construct the insertLinkButton html
+ html = get_plugin_html()
+ //add the button to the tools box
+ jQuery(wym._box)
@digi604
digi604 Aug 13, 2012 Member

I find in this file 3 different JQuery definitions: CMS, $ and JQuery. Is there a need for this inconsistency?

@digi604 digi604 commented on the diff Aug 13, 2012
...ter/templates/cms/plugins/twitter_recent_entries.html
});
-});
+})(window.CMS);
@digi604
digi604 Aug 13, 2012 Member

Why is the file above namespaced with window.CMS.$ and this one with window.CMS?

@andrewschoen
Contributor

I fixed the consistency issues between twitter and wymeditor templates. Both should pass window.CMS into the closure, use CMS.$ to start the document.ready and then just use $ inside of that.

Thanks for the review!

@travisbot

This pull request passes (merged 9e34aea into c23c632).

@digi604
Member
digi604 commented Aug 14, 2012

Ok i think this is ready for merge. Which tickets can be closed after this merges?

@piquadrat
Contributor

AFAIK, #1349 and #1350.

@digi604 digi604 merged commit 2fc9821 into divio:develop Aug 14, 2012

1 check passed

default The Travis build passed
Details
@coveralls

Coverage Status

Changes Unknown when pulling 9e34aea on andrewschoen:changelist-bug-js-namespacing into * on divio:develop*.

@coveralls

Coverage Status

Changes Unknown when pulling 9e34aea on andrewschoen:changelist-bug-js-namespacing into * on divio:develop*.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment