The create/VIE/Backbone port is taking place in parallel in the D7 and D8 versions of this module.

D7 - branch: node/1808076-createjs-d7 - this issue
D8 - branch: node/1824100-createjs-d8 - #1824100: Convert edit module JavaScript to use Create.js, VIE, and Backbone (D8)

The difference between the two branches should boil down to how the libraries are included. We're pushing for create.js and VIE to be included in D8 Core. For D7, we need to include these libraries in the module.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

WOOOOOOHOOOOO!

Wim Leers’s picture

Also, please take a look at https://github.com/wimleers/edit-createjs while working on this. Specifically the Backbone.sync implementation that started in https://github.com/wimleers/edit-createjs/commit/cc913db4b61d9c1bf7f22e7... and has been improved in later commits.

frega’s picture

yay, indeed :) I've merely started refactoring the ajax stuff, so before we arrive at a proper Backbone.sync implementation, a lot remains to be done! Some architectural changes might be also advisable (e.g. the chain of control between backbone views and createjs editables/jqueryui widgets seems a little topsy-turvy).

FWIW, I "rebased" the module on alpha6 of the spark distro today :)

Wim Leers’s picture

I noticed :) Thanks so much for your hard work, @frega! Much appreciated!

jessebeach’s picture

FileSize
157.94 KB

Alright, let's start patching. I've been looking through the following two repos, the first to find inspiration for the Backbone port in general

git://github.com/wimleers/edit-createjs.git

and the second for specific changes that @frega made to views.js and ui-editables.js

git://github.com/frega/edit-createjs.git

I've gotten the Backbone port to a place where we can start wiring in the changes that @frega put together. The big problem is the edit.js file in the github repo is very different from the edit.js file here. It assumes the presence of create.js and vie.js to do a lot of page processing. For this patch, we have to assume that we don't have create.js and vie.js.

So my approach has been to port edit.js to Backbone with small changes, looking to the github repo code for guidance, but not so much for copy-paste. I also want to move us away from dumping properties into Drupal.edit.* and towards a more API-like interface to the Edit module front end code.

I've set up a singleton pattern for the app. A singleton is an instance of a Class that can only have one instance. So any part of the Drupal code can invoke a new instance of the Edit application and they'll get the singleton. Until we get an AMD architecture, this will allow us to break the application down into modules the extend the Edit application through a lightweight API instead of simply dumping variables, objects and functions into Drupal.edit.*. Ultimately we'll be able to store more state in the application rather than the DOM and do less traversing of the DOM as a consequence.

...
/**
 * The Edit application Model.
 */
var EditApp = Backbone.Model.extend({
  ...
});

// Define a variable to hold the application instance. This is kept out of
// scope of the Edit object so that it is immutable once invoked.
var application;

/**
 * The in-place-edit controller.
 */
var Edit = {
  /**
   * The application is a singleton.
   */
  getApplicationInstance: function (options) {
    // Check for an existing instance of EditApp.
    if (application !== undefined) {
      // Set additional options on the instance.
      if (options !== undefined && typeof options === 'object') {
        var opt;
        for (opt in options) {
          if (options.hasOwnProperty(opt)) {
            application.set(opt, options[opt]);
          }
        }
      }
      return application;
    }
    // Create a singleton reference EditApp Model.
    return application = new EditApp(options || {});
  },
  /**
   * Add properties to the application instance.
   */
  extend: function (extensions) {
    if (application !== undefined && extensions !== undefined && typeof extensions === 'object') {
      var ext;
      for (ext in extensions) {
        if (extensions.hasOwnProperty(ext)) {
          EditApp.prototype[ext] = extensions[ext];
        }
      }
    }
  }
}
...
// Assign the Edit controller to the global Drupal namespace.
Drupal.Edit = Edit;

The application is invoked with the getApplicationInstance method.

var app = Edit.getApplicationInstance(Drupal.settings.edit);

What I'm working on now with @frega's code is how we break the app down into modules and building Models for the editables.

I still need another day to wrap up this part of the work, but I wanted to get a patch up now and give you the opportunity to chime in now before I go too crazy with the refactoring.

jessebeach’s picture

FileSize
153.11 KB

Just realized that the patch in #5 wasn't squashed into a single commit against the 7.x-1.x branch.

Rerolled. Should be easier to read now.

Wim Leers’s picture

Nice :)

I agree with your overall line of thought, but I don't see much benefit in having a highly abstracted "set()" method on your singleton. You're now doing this.set('fields', fields), why not simply this.fields</code? What's so bad about just having <code>this.fields; this.whatever;?
I don't *mind*, but it does feel like overengineering and abstracting where abstracting is pointless.
*Unless* you say this is to be able to always just do this.get('whatever') (thanks to the fact that you use $.proxy everywhere) instead of what I had to do: Drupal.edit.whatever, i.e. to make the code more legible.

In any case, I trust your judgement. You don't need me to review this, unless you just want feedback :)

+++ b/js/edit.jsundefined
@@ -1,145 +1,273 @@
+(function ($, _, Backbone, undefined) {

@nod_ says we shouldn't have undefined in here IIRC.

+++ b/js/edit.jsundefined
@@ -1,145 +1,273 @@
+    getModel: function () {
+      return this.models;

This looks weird?

+++ b/js/ui-editables.jsundefined
@@ -8,127 +8,30 @@
+    var entity = Drupal.edit.vie.entities.get(Drupal.edit.util.getElementSubject($editable));

This is to facilitate migration to use VIE.js in the future?

+++ b/js/ui-editables.jsundefined
@@ -8,127 +8,30 @@
-      setTimeout(function() {
-        Drupal.edit.toolbar.addClass($editable, 'info', 'loading');
-      }, 0);
+      // Indicate in the 'info' toolgroup that the form is loading.
+      // Drupal.edit.toolbar.addClass($editable, 'primary', 'info', 'loading');
+      toolbarView.addClass('info', 'loading');

The setTimeout() call here (and elsewhere) is there for a reason: to make sure CSS transitions actually work.

+++ b/js/ui-editables.jsundefined
@@ -152,10 +56,73 @@ Drupal.edit.form = {
+    var onLoadCallback = function(status, $form) {

This is the stuff @frega wrote, right?

+++ b/js/ui-editables.jsundefined
@@ -165,14 +132,138 @@ Drupal.edit.form = {
+    var element_settings = {
+      url      : Drupal.edit.util.calcFormURLForField(edit_id),
+      event    : 'edit-internal.edit',
+      $field   : $field,
+      $editable: $editable,
+      submit   : { nocssjs : ($field.hasClass('edit-type-direct')) },
+      progress : { type : null } // No progress indicator.
+    };
+    // Removing existing Drupal.ajax-thingy.
+    if (Drupal.ajax.hasOwnProperty(edit_id)) {
+      delete Drupal.ajax[edit_id];
+      $editable.unbind('edit-internal.edit');
+    }
+    Drupal.ajax[edit_id] = new Drupal.ajax(edit_id, $editable, element_settings);

For more info about this special piece of Drupal Hell: #1653142: Improve Drupal.ajax.

+++ b/js/ui-editables.jsundefined
@@ -165,14 +132,138 @@ Drupal.edit.form = {
+  saveForm: function (vieEntity, predicate, $editable, value, callback) {

More @frega code, right?

+++ b/js/ui-editables.jsundefined
--- /dev/null
+++ b/js/views.jsundefined

This again seems like a @frega creation, with e.g. the ToolbarView added by you?

jessebeach’s picture

the set/get methods are Backbone Model methods.

http://backbonejs.org/#Model-set

Set triggers change events when Model instance attributes change.

Wim Leers’s picture

Erh… DUH! Now that makes sense :) I didn't notice it was Backbone.Model.set() — stupid me :/

jessebeach’s picture

I merged in changes from the frega repo. I also modified library declarations in hook_library and packed backbone.js and underscore.js into the moduel for now. We'll be removing them and referring to the Core versions when #1149866: Add Backbone.js and Underscore.js to core is committed.

I'm now working on porting changes from #6 to this updated patch. I'll have an updated patch in a few hours.

Active work has moved to the node/1808076-createjs branch.

jessebeach’s picture

So, building VIE (using the build.xml and ant) without the Version 1.0 compatibility units drops the minified build size down from 71K to 33K. This is for the non-ALL version that does not include jQuery, etc. That's a 46% drop in size for math lovers. We've currently got the kitchen-sink 241K version in the patch in #10.

I'll propose a change request to include a build file with just the 2.0 code to @henribergius against his dev repo.

frega’s picture

I quickly skimmed the merge - i am sorry it's such rebase hell :| I noticed that there's a duplicate Drupal.edit.form in ui-editable.js. The first one can be removed entirely: it's the vestigial Drupal.edit.toolbar-code that has already been refactored to the Drupal.edit.views.ToolbarView in views.js. HTH!

jessebeach’s picture

That was the one part of the merge that I kept the HEAD bit, because I wasn't sure if it would be needed. I'll pull it now.

Also, the VIE build at 33K gzips down to 10K. Not bad. As @nod_ noted, smaller than Backbone/Underscore.

jessebeach’s picture

This updated patch addresses the issues in #12. I had to kinda merge the two instances of Drupal.edit.form to resolve a few JS errors when attempting to save a form. The interdiff shows the changes between #10 and #14.

frega’s picture

To complete the merge attached patch needs to be applied (against node/1808076-createjs). It re-adds some variables and references (to the ToolbarView instance/$toolbar-Element) and removes a lingering call to Drupal.edit.toolbar.

Yay - this kind works for me now (had to update/git aloha.module on the server-side - otherwise there's a fatal php error).

Issues/bugs outlined on github regarding functionality still apply, though, and there is a UI-regression regarding the positioning of the "Field Info"-Block.

PS. I'll open another issue because i think, before proceeding we should consider splitting up the js files and use grunt (or something like that) to have a proper toolchain and build process available.

jessebeach’s picture

@frega, I added you as a maintainer with VCS access. Let's work in the node/1808076-createjs branch. We'll merge everything to 7.x-1.x and propose a D8 patch when we've reached consensus that the create.js/VIE stack is the sanctioned approach to including inline editing in Core for D8.

Go ahead and commit your changes in #15 to the branch. Also, the PHP fatal when calling $function() (you commented this out in your patch) might be avoided by checking the function call first in an is_callable() if statement.

frega’s picture

Committed #15 (minus the edit.module hack) in 135ed01.

Wim Leers’s picture

Component: Code » Backbone.js
frega’s picture

FileSize
10.97 KB

As just agreed on in IRC, please find patch doing some "dead code culling" attached

jessebeach’s picture

Title: Convert edit module JavaScript to use Backbone.js » Convert edit module JavaScript to use Create.js, VIE, and Backbone (D7)
jessebeach’s picture

Issue summary: View changes

Added the feature branch name

jessebeach’s picture

Issue summary: View changes

updated the issue summary with branch info.

jessebeach’s picture

Issue summary: View changes

added version info to the branch names

jessebeach’s picture

I committed the patch in #19 to node/1808076-createjs-d7.

frega’s picture

Spent the day cleaning up the D7 branch a little more still.

- Addresses #1823200 ("Dirty/Changed" State for editable/FieldView)
- Drupal.edit.log helper (removing console.log), Drupal.edit.confirm polyfill for (async) Confirm-Dialog
- Removed dead "checkHighlight"-call
- Decoupling ToolbarView and making it listen on events in the parent view
- Add Backbone.Router and a Drupal.edit.views.MenuView to handle the "View" / "Quick Edit" state (adding #quick-edit-URL)
- Further refactoring ui-editables.js moving things slowly towards Drupal.edit.views.FormEditableFieldView

Wim Leers’s picture

Ugh. I spent most of the day in the D8 branch (i.e. #1824100: Convert edit module JavaScript to use Create.js, VIE, and Backbone (D8)). I'm assuming it'll be easy to port to D8? :)

Wim Leers’s picture

Ported and committed @frega's patch in #22. "Dirty/Changed" is currently not working for type=form editables through.

I found and fixed one bug in the patch: Drupal.edit.views.FieldView should have this to make the form loading indicator work on type=direct editables:

+    showLoadingFormIndicator: function() {
+      // Trigger this event to propagate to the appropriate ToolbarView.
+      this.trigger('showLoadingFormIndicator');
+    },

http://drupalcode.org/project/edit.git/commit/fd22278

Wim Leers’s picture

Status: Active » Fixed

Work on D7 is temporarily on hold, we've started from the work done here, ported it to D8, refactored it A LOT, made upstream Create.js contributions that facilitated better code on our end, etc. That work will be backported to D7.

See #1824100-2: Convert edit module JavaScript to use Create.js, VIE, and Backbone (D8).

webchick’s picture

Title: Convert edit module JavaScript to use Create.js, VIE, and Backbone (D7) » Convert edit module JavaScript to use Create.js, VIE, and Backbone
Version: 7.x-1.x-dev » 8.x-1.x-dev

Fixing some metadata here to make it clear that this applies to D8 only, for now.

Wim Leers’s picture

Version: 8.x-1.x-dev » 7.x-1.x-dev

#26: this was all actually pioneered by @frega in a branch that was forked off of 7.x-1.x. We then moved all that to D8 and continued working there. So this issue is specifically about "Edit on Create.js in D7", the issue for "Edit on Create.js in D8" is this one: #1824100: Convert edit module JavaScript to use Create.js, VIE, and Backbone (D8). I know, it's confusing :/

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

bolded the branch names