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).

Comments

jpetso’s picture

Cool 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!

sepgil’s picture

StatusFileSize
new17.58 KB

Actually 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.

jpetso’s picture

Looks good. I need to merge my variable-inputs patch first (nearly done), then I'll look into merging this one.

jpetso’s picture

StatusFileSize
new15.53 KB

Some notes for future patches, and remaining issues that I noticed in the current patch:

  • Try *very* hard not to use global variables, in PHP as well as in JavaScript. If you absolutely must have some, use the Drupal.[moduleName].* namespace. A global variable named "elements", for example, is just unacceptable.
  • Write comments in commanding style, e.g. "Save all elements in a global array." instead of "Saves all elements in a global array."
  • Use a text editor that displays trailing spaces on a code line. Trailing spaces are evil.
  • Use a text editor that displays the 80-characters line width. Code in general, and comments in particular, should not exceed the 80 character width except if necessary for good readability.
  • For strings, stay consistent for the module's preferred way of quoting: my modules always use single quotes instead of double quotes wherever possible, except if the string contains single quotes itself.
  • Prototype (or function) names like Drupal.transformationsUi_Grippie look awful. I'll figure out how to improve that, maybe I can just write Drupal.transformationsUi.Grippie and it'll still work. (I'm not the big JavaScript expert here.) On the other hand, I don't feel like experimenting with JS today, so I'll just go with Drupal.transformationsUiGrippie and be done. Anyways, never mix underscores and camel casing in a single variable name. That's awful.
  • There's already the .transformations-operation-blocks class, no need to introduce a new #transformations-operation-editPanel id for the same element. (Also, camel casing isn't very good looking there when all other CSS classes use hyphens for the separation of word parts.) For the moment, I made the .transformations-operation-blocks class into an id for easier porting, but I'll investigate if a class isn't sufficient too. (Why use ids when classes can do the same job but allow multiple occurrences on a page.)
  • transformations_pipeline_edit.js is not an ideal name, because: 1. it's not part of transformations but of transformations_ui, and the filename lacks the "ui" part, and 2.
  • You drupal_add_js()'d the transfomations_operations.js file even though it otherwise disappeared from the patch. Please be careful about cleaning up obsolete code.
  • Also regarding proper clean-ups, I couldn't find any reference to the .transformations-ui-pipeline-edit class that you introduced in the CSS file. Neither the current code nor any code added by your patch seems to assign that class to an HTML element. I'm also not sure why some of the changes are necessary (e.g. assigning fixed width to elements - EEEVIL!! - or "position: relative;" for the canvas and block tables. There's also no explanation on why the changes have been done. Thus, I'm skeptical about the CSS changes, and will only merge them manually as I find them appropriate or necessary. Explanations on why stuff is necessary are still appreciated.
  • The transformations_ajax_get_position() callback shouldn't be necessary, and will cause unnecessary performance issues when a pipeline has lots of operations. Better to just include the position in Drupal.settings too, we can then read it from there without further callbacks.
  • As stated before, I want the improved layout (slot label being a link to the "Assign fixed value" form, submit button being just a small rectangle on the edge of the operation block) in the non-JS version too.
  • Oh, and do mind correct spelling. Some examples: Spelling grippie "grippie" in one place and "grippey" in another is not ideal. "alle" instead of "all" is a germanism. Words in the middle of a sentence usually begin with a lower-case character, except for names ("Eigennamen") which preserve the original spelling. See the updated patch for the complete list.
  • Your Drupal.behaviors.transformationsUiConnections uses the same name as the existing Drupal.behaviors.transformationsUiConnections, which means one of them will be overwritten and thus non-functional.
  • The links in the JavaScript-added <a> elements are relative instead of absolute, I wonder if that might be an issue in certain circumstances. Need to further investigate.
  • Maybe we can indeed do without a strict jQuery UI dependency. For the time being, let's try to make this functionality optional with a module_exists() check.

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.

jpetso’s picture

Status: Needs work » Fixed
StatusFileSize
new30.78 KB

Aaaand... 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 )

Status: Fixed » Closed (fixed)

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