Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove admin/js/jquery.init.js from SortedCheckboxSelectMultiple #149

Merged
merged 1 commit into from
Mar 5, 2020

Conversation

Xyene
Copy link
Contributor

@Xyene Xyene commented Oct 20, 2019

jquery.init.js seems to be used by Django's admin templates to remap $ into django.jQuery, and then remove $ from the global scope. Its contents are:

var django=django||{};django.jQuery=jQuery.noConflict(true);

(See https://api.jquery.com/jQuery.noConflict/.) This means that including a SortedCheckboxSelectMultiple widget on a page that expects jQuery to be accessible under $ breaks code on that page.

This commit removes the loading of jquery.init.js, which fixes this issue. The existing code does not appear to be relying on its behaviour, and it was introduced recently in 19a11c9 (without explicit justification that I could find).

`jquery.init.js` seems to be used by Django's admin templates to remap `$` into `django.jQuery`, and then _remove `$` from the global scope_. Its contents are:

```
var django=django||{};django.jQuery=jQuery.noConflict(true);
```

(See https://api.jquery.com/jQuery.noConflict/.) This means that including a `SortedCheckboxSelectMultiple` widget on a page that expects jQuery to be accessible under `$` breaks code on that page.

This commit removes the loading of `jquery.init.js`, which fixes this issue. The existing code does not appear to be relying on its behaviour, and it was introduced recently in 19a11c9 (without explicit justification that I could find).
Xyene added a commit to DMOJ/online-judge that referenced this pull request Oct 26, 2019
A PR has been opened upstream at jazzband/django-sortedm2m#149, but seems unacknowledged at the moment.
@Xyene
Copy link
Contributor Author

Xyene commented Nov 4, 2019

Just wanted to confirm that the removed JS does not seem to be necessary even on 2.2 (to which we have upgraded), and that the CI failure seem to be unrelated to this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants