This is the first patch for the new ui.
It currently lets you drag the operations arround and saves the position.
You can also drag the operation outputs and drop it on a input to connect two operations. To delete the connection, you need to click on the output. If click on an input, you can assign a fixe value for it.
Currently the ui reloads the whole site, when something was done(like saving, connecting, ...).
The ui depends on the jQuery UI and on the jQuery update module (because it uses jQuery UI 1.7.2) .
I couldn't make the editPanel (where the operations lie) resizable, because grippie is just for textareas and the jQuery UI module currently doesn't support themes(jQuery has resizable functionality, but it needs the icons from a theme).
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | transformations-dragndrop-final.diff | 30.78 KB | jpetso |
| #4 | transformations-dragndrop.diff | 15.53 KB | jpetso |
| #2 | issue_transformations_ui_v003.patch | 17.58 KB | sepgil |
| issue_transformations_ui.patch | 17.52 KB | sepgil |
Comments
Comment #1
jpetso commentedCool stuff. Here's a few comments based on a quick code review (not yet tested).
Functional issues:
- The newly introduced "connection span" is not functional without JavaScript. Please try to make the "Assign fixed input" link (see further above in transformations_ui.pipeline.edit.inc) into a single link, and adapt the current input/output submit button so that it just takes "connect" functionality but not "assign fixed value". (Ideally, it would even be an image button instead of a standard submit button, also see the image buttons further up in the file for an example.) Then, you can replace the button with a span that displays the same image. In fact, I can live without the image, but a graceful non-JS fallback would still be nice :]
- The wildcard ("%") in menu paths like 'build/transformations/ajax/connect/%' only stands for a single parameter (in this case, parameter no. 4). You either want to provide wildcards for all parameters (e.g. "4, 5, 6, 7, 8", which would make 'build/transformations/ajax/connect/%/%/%/%/%') or no wildcards at all ('build/transformations/ajax/connect'). As Drupal's menu system can only handle up to 8 parameters and parameter no. 8 is already the ninth one, please go with the latter. You can even remove the 'page arguments' option, as Drupal hands all parameters to the callback that are not specifically mentioned in the menu path information. The function signatures stay the same still.
- When the module depends on the jquery_ui module, you should specify that in the transformations_ui.info file, just like the other dependency that's already in there ("transformations", that is).
Coding style issues:
- I doubt you wanted to include the ".buildpath" and ".project" files in the patch.
- The JavaScript functions use pretty generic names in the global namespace, this could easily clash with other people's JS code. Please use "Drupal." as namespace and prefix function names with "transformationsUi".
- transfomations_operations.js can be merged with transformations_pipeline_edit.js. And while we're at it... both of those are small enough to be merged with the current .js file, too.
- You don't want to include "?>" at the end of a PHP file (.ajax.inc), and you definitely don't want any patch to introduce a "\ No newline at end of file" notice.
- No trailing spaces after a line of code. No spaces at all in empty lines.
- Comments with sentences include a space after the "//" and end with a dot at the end of the sentence. Line comments also got a space before the "//" in case they're in the same line as other code.
- Typo: "Determans whether..." → "Determines whether...".
- Coder won't let you go through with lines like "if($keyType=='input'){" (spaces missing) or "}else{" (spaces missing, and the else belongs on a different line). Please try to use single spaces between most elements (also in "$jsElement = array('key'=>$key, 'keyType'=>$keyType);"). Granted, I haven't checked my own code with Coder for a while, so I'd better make sure it passes before criticizing new code. I'll make sure to adapt everything to my likings when merging the code because I'm extra picky, but please try to follow Drupal coding standards whenever you can.
Most of the coding style issues are minor nitpicks, while the overall code is cool - please don't be discouraged by those, I'm just a little careful about how my module looks and reads. Thanks a lot for the patch, hopefully we'll be able to get it merged soon!
Comment #2
sepgil commentedActually i don't need the connection spans any more, since i replaced all submit buttons with spans(with JS). Now you drag the output itself onto the input(like i had it at the beginning).
The input link points now directly to the fixed value form. I corrected all those little things(like parameters, style code issues,...) and I omplemented the resizable editPanel. It looks like the grippie from Drupal, but works with jQuery UI resizables.
Comment #3
jpetso commentedLooks good. I need to merge my variable-inputs patch first (nearly done), then I'll look into merging this one.
Comment #4
jpetso commentedSome notes for future patches, and remaining issues that I noticed in the current patch:
I'll take care of all of these issues (partly done) and post the final patch when I'm done. The attached patch is what I've got at the moment.
Comment #5
jpetso commentedAaaand... committed! I improved operation block layout and workflow for non-JS as well, which makes for slightly more PHP code in the patch but on the other hand reduces the JavaScript by great lengths, and common approaches are always a good thing.
I also revised some of the other approaches and addressed all of the bullet points I listed above, and even designed a connection icon (a circle! yay.) for use as draggable element - but even without that, it still looks good. I fixed some bugs (holy resize handle, Batman! and non-positioned elements properly store the position even after the first time they are dragged.), improved drop targets ("tolerance: 'pointer'", plus can now be dropped anywhere on the slot) and equipped drag&drop functionality both ways (input <-> output) instead of just from inputs to outputs. Disconnecting slots is done similar to before, with two sequential clicks on the connector icon. Lastly, the resize handle now doesn't show up when drag&drop functionality is not enabled.
This thing is now so cool that it even works in Konqueror. It still has a few shortcomings I guess, including the non-existence of a check that prevents an operation's output slot to be dragged onto the same operation's input slots. If I'm correct, URL encoding of operation ids ("entity" variable) and keys is also not done in the JavaScript code, we'll probably run into problems when operations like the pipeline operation provide keys that include colons, or maybe slashes, or whatever.
jQuery UI 1.7 requires jQuery Update 6.x-2.x, and that's a major downer because the maintainers haven't even released a pre-release on the 2.x branch yet. What are the exact improvements in jQuery UI 1.7 compared to 1.6 that are required for this functionality? I'd really like to offer the JavaScript functionality to people that are not living on the bleeding edge, would it be possible to downgrade/backport the drag&drop code to jQuery UI 1.6 so we only need to depend on the recommended version of jQuery Update?
Anyways, those are minor issues. The big issue is solved, the patch is in CVS, and Transformations one step further on its way to world domination. I'll attach the "final" patch here for reference, maybe you're interested in how it changed before being committed.
Thanks a lot for your contribution! (hoping to see more patches from you! :D )