Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#22 | 1808076-more-refactoring-cleanup-22.patch | 25.1 KB | frega |
#19 | cleanup-editcreatejs.patch | 10.97 KB | frega |
#15 | 1808076_port-to-createjs_mergeissues_15.patch | 3.04 KB | frega |
#14 | 1808076_port-to-createjs_14.patch | 478.04 KB | jessebeach |
#14 | interdiff.txt | 5.43 KB | jessebeach |
Comments
Comment #1
Wim LeersWOOOOOOHOOOOO!
Comment #2
Wim LeersAlso, 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.
Comment #3
frega CreditAttribution: frega commentedyay, 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 :)
Comment #4
Wim LeersI noticed :) Thanks so much for your hard work, @frega! Much appreciated!
Comment #5
jessebeach CreditAttribution: jessebeach commentedAlright, 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 application is invoked with the
getApplicationInstance
method.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.
Comment #6
jessebeach CreditAttribution: jessebeach commentedJust 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.
Comment #7
Wim LeersNice :)
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 simplythis.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 :)
@nod_ says we shouldn't have undefined in here IIRC.
This looks weird?
This is to facilitate migration to use VIE.js in the future?
The setTimeout() call here (and elsewhere) is there for a reason: to make sure CSS transitions actually work.
This is the stuff @frega wrote, right?
For more info about this special piece of Drupal Hell: #1653142: Improve Drupal.ajax.
More @frega code, right?
This again seems like a @frega creation, with e.g. the ToolbarView added by you?
Comment #8
jessebeach CreditAttribution: jessebeach commentedthe set/get methods are Backbone Model methods.
http://backbonejs.org/#Model-set
Set triggers change events when Model instance attributes change.
Comment #9
Wim LeersErh… DUH! Now that makes sense :) I didn't notice it was Backbone.Model.set() — stupid me :/
Comment #10
jessebeach CreditAttribution: jessebeach commentedI 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.
Comment #11
jessebeach CreditAttribution: jessebeach commentedSo, 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.
Comment #12
frega CreditAttribution: frega commentedI 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!
Comment #13
jessebeach CreditAttribution: jessebeach commentedThat 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.
Comment #14
jessebeach CreditAttribution: jessebeach commentedThis 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.Comment #15
frega CreditAttribution: frega commentedTo 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.
Comment #16
jessebeach CreditAttribution: jessebeach commented@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 anis_callable()
if statement.Comment #17
frega CreditAttribution: frega commentedCommitted #15 (minus the edit.module hack) in 135ed01.
Comment #18
Wim LeersComment #19
frega CreditAttribution: frega commentedAs just agreed on in IRC, please find patch doing some "dead code culling" attached
Comment #20
jessebeach CreditAttribution: jessebeach commentedComment #20.0
jessebeach CreditAttribution: jessebeach commentedAdded the feature branch name
Comment #20.1
jessebeach CreditAttribution: jessebeach commentedupdated the issue summary with branch info.
Comment #20.2
jessebeach CreditAttribution: jessebeach commentedadded version info to the branch names
Comment #21
jessebeach CreditAttribution: jessebeach commentedI committed the patch in #19 to node/1808076-createjs-d7.
Comment #22
frega CreditAttribution: frega commentedSpent 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
Comment #23
Wim LeersUgh. 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? :)
Comment #24
Wim LeersPorted 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:http://drupalcode.org/project/edit.git/commit/fd22278
Comment #25
Wim LeersWork 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).
Comment #26
webchickFixing some metadata here to make it clear that this applies to D8 only, for now.
Comment #27
Wim Leers#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 :/
Comment #28.0
(not verified) CreditAttribution: commentedbolded the branch names