ic-labs / django-icekit

GLAMkit is a next-generation Python CMS by the Interaction Consortium, designed especially for the cultural sector.
http://glamkit.com
MIT License
47 stars 11 forks source link

Remove the need for case-by-case workarounds for content plugins that use javascript #138

Closed cogat closed 7 years ago

cogat commented 8 years ago

The Image plugin, for example, has a raw_id and a collapse class, both of which require javascript to function.

On a pre-saved content plugin, the js is instantiated and everything works normally.

But if you add a content plugin, the JS isn't initialised and the raw_id magnifier icon functions as a navigate-to-page link. And the collapse control doesn't function.

This is partly fixed by making the admin template for the content extend icekit/admin/fluent_layouts_change_form.html, which includes a block that reinitialises raw_ids in particular.

But this solution doesn't address the collapse control, or any other javascript that a content plugin may have.

It would be great to figure out how to initialise the same javascript when a content plugin is 'added' dynamically as when it's loaded from a saved model.

jmurty commented 8 years ago

There are a number of things going on here, all of which will need to be made to line up exactly for us to fix this issue once and for all. I think I have the solution, but putting it in place will take more work.

Background

Pretty much all the custom dynamic behaviour implemented in JS is bind'ed to DOM elements as part of the document load processing e.g. in $(document).ready() blocks. This makes it near impossible to re-apply these bindings dynamically after the initial page load without completely rewriting all the relevant scripts.

Luckily we shouldn't need to do this because the form when first loaded includes hidden template fragments in the DOM to be copied into place when the Add button is clicked, along with some class and ID manipulations that also need to be performed to keep things straight.

This means that the content template fragments already have all the event bindings they need. The problem is that the events get lost during the copy-into-place process.

Copy templates with event listeners

To make the dynamic behaviour work we need to copy the content item template fragments into place in such a way that the existing event listeners are included.

jQuery has a clone() method that does exactly this, as long as you include a true argument for the extra withDataAndEvents option: https://api.jquery.com/clone/

So we should be able to clone() the existing template, re-insert the cloned copy into the right place in the page, and be good to go.

There is, of course, an extra wrinkle. After experiencing much confusion testing this clone(true) approach and finding that it did not work, I eventually figured out that event listeners bound to DOM objects by jQuery are recorded internally by that instance of jQuery and are not available to other instances of jQuery. In other words, event bindings created by the Django admin's version of jQuery django.jQuery are not visible to, and cannot be cloned by the non-admin version of jQuery $ generally also included on the site. Thanks to the last commenter here for this detail I wasn't aware of: https://github.com/jquery/jquery/issues/1973

With this knowledge I was finally able to perform successful manual tests in the browser console to clone an Image content item's empty form template into place on a LayoutPage form with working dynamic behaviour. Here's the incantation to do the clone and remove a CSS class that otherwise hides the cloned form:

django.jQuery("#placeholder-fs-0 .cp-content").append(
    django.jQuery("#imageitem-empty").clone(true).removeClass("empty-form")
)

Next steps

To apply this fix for Fluent Contents in general we need to fix the existing behaviour to do the full clone with events.

The Add button's copy-into-place implementation is in Fluent Contents' _fluent_contents/static/fluent_contents/admin/cpplugins.js JS file, with a function defined as cp_plugins.add_formset_item.

This implementation currently copies template content using either standard Javascript HTML manipulation methods or jQuery's plain clone() without the extra true argument to include events, depending on the code path taken. The implementation also does not use the same django.jQuery Django admin instance of jQuery as is used to bind the events on page load, which means that simply doing a clone(true) in that code is not enough to also clone the events -- believe me, I tried.

This means that to fix the problem we need our own customisation of _fluent_contents/static/fluent_contents/admin/cpplugins.js that always and only uses clone(true) when copying template fragments into place, and does so using the django.jQuery instance of jQuery.

Here is a working example of the necessary change, from around line 352 of that file:

// Clone the item.
var new_index = total.value;
var item_id   = inline_meta.prefix + "-" + new_index;

// EXISTING APPROACH LOSES EVENT BINDINGS
//var newhtml   = options.get_html
//              ? options.get_html(inline_meta, new_index)   // hook
//              : inline_meta.item_template.get_outerHtml().replace(/__prefix__/g, new_index);
//var $newitem  = $(newhtml).removeClass("empty-form").attr("id", item_id);

// NEW (PARTIAL) APPROACH TO INCLUDE EVENT BINDINGS
var $newitem = django.jQuery(inline_meta.item_template).clone(true);
$newitem.removeClass("empty-form").attr("id", item_id);

// Add it
pane.content.append($newitem);
pane.empty_message.hide();
jmurty commented 7 years ago

NOTE: The changes to django-fluent-contents' _cpplugins.js file are also made in a branch of our fork here: https://github.com/ixc/django-fluent-contents/tree/feature/fix-admin-content-plugins-js-behaviour Once we are sure this works for us we can submit the changes there as a PR against the upstream project.

cogat commented 7 years ago

@jmurty this is ready to merge. Let me know which branch I should include in ACMI.

jmurty commented 7 years ago

This fix is now merged into ICEKit's "develop" branch. If/when the upstream pull request against django-fluent-contents is accepted we can delete our slightly-customised copy of the the _cpplugins.js file.