Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gifad’s picture

Status: Active » Needs review
FileSize
988 bytes

The attached patch allows user to double click the image of an atom in the library widget, to insert it at the last insertion point defined in the active ckeditor instance.

gifad’s picture

Title: Use double click in addition to drag and drop » Use double click and/or Insert button in addition to drag and drop
FileSize
2.17 KB

Go a step farther, and finally insert an “Insert” button...

PS: This patch was generously sponsored by the Image Assist D6 Veterans Association ;-)

jcisio’s picture

Component: User interface » Library/DnD
Status: Needs review » Needs work

Patch looks good. A small add-on: will we be able to make it work without CKEditor (a plain textarea)?

gifad’s picture

Status: Needs work » Needs review
FileSize
3.74 KB

A small add-on ?
I could not find the equivalent of “ckeditorInstance” for plain textareas, so I had to make it myself...
Then, I realized “insertHtml” is a ckeditor function, so I had to make it myself again (well, with google's help...)
So I'm not sure that “Patch looks good.
But it works ;-)

jcisio’s picture

Title: Use double click and/or Insert button in addition to drag and drop » Altenatives of drag and drop to insert atoms: double click and/or Insert button
Status: Needs review » Needs work

Well, coding standards are not respected in copied snippet. Also, I think you can use Drupal.getSelection (it is used nowhere in core and is removed in D8, but it is still there in D7) to make your code shorter. Besides, we try to not introduce global variables/functions, put them in Drupal.dnd for example. Then we are good. This is a useful feature, because currently dnd does not work well with touch screens (the touch events).

gifad’s picture

Status: Needs work » Needs review
FileSize
3.71 KB

I don't know how to use Drupal.getSelection to make my code shorter, but fixed the other issues.

BTW, if you “try to not introduce global...”, you could add this to the patch :

@@ -263,9 +273,10 @@ renderLibrary: function(data, editor) {
 
   // Preload images in editor representations
   var cached = $.data($(editor), 'dnd_preload') || {};
+  var editor_id;
   for (editor_id in Drupal.dnd.Atoms) {
     if (!cached[editor_id]) {
-      $representation = $(Drupal.dnd.Atoms[editor_id].editor);
+      var $representation = $(Drupal.dnd.Atoms[editor_id].editor);
       if ($representation.is('img') && $representation.get(0).src) {
jcisio’s picture

Status: Needs review » Needs work
FileSize
3.49 KB

New patch:
- Refactoring and move functions into Drupal.dnd (so that it could be overriden if necessary).
- Use editor.insertElement instead of editor.insertHtml (thanks to StackOverflow) so that we can remove the <p>&nbsp;</p> hack. I don't modify current code BTW.
- Change the "insert" link name/class to avoid collision.

I only tested with Chrome/Linux so I think it would be tested more. Bugs that need to be fixed:
- Can not insert into a textfield.
- Do not use HTML markup, use SAS instead when insert into textarea/textfield.

Minor bug, but nice to eliminate:
- When the cursor is in the legend area, it creates "atom inside atom".

gifad’s picture

I won't review your patch tonight, but i'll comment two of your comments:

  • “Can not insert to textfield” : You requested “make it work without CKEditor (a plain textarea)?”, so I coded " $('textarea').focus(function(){”; should duplicate with textfield ?
  • “it creates "atom inside atom".” : it's not a bug, it's a (nice) feature; I tested it intensively : it works at any dimensions, any depth, kind of "atoms panels" (you need appropriate contexts, of course)
    I did not mentioned it, as the legend is reasonably editable only in wywiwyg mode, not in the awful "Edit atom properties".

BTW, I'm working on a “addRef” link, to insert atom in a Atom Reference Field (the one with “Drop a ressource here”) : One of the problems is the space left in the library widget : use icon buttons ? CTools buttons ?

jcisio’s picture

For text input, we don't have to duplicate. I believe something like $('textarea, input[type="text"]') would work. Needs test BTW, because type="text" is the default, so we have to take care of the case where there is no "type", too.

About the "bug" (or feature...), I don't care to fix it right now. It is left outside the "bugs that need to be fixed" group. Maybe it is an expected feature. But yesterday when I tested, I clicked "insert" once, one atom was inserted and the caret was placed in the legend. I clicked "insert" again for another atom, then it was placed in the legend. The expected behavior is to place it next to the other atom.

jcisio’s picture

Title: Altenatives of drag and drop to insert atoms: double click and/or Insert button » Alternatives of drag and drop to insert atoms: double click and/or Insert button
gifad’s picture

Legend “unattended” processing works far better with the alternate legend layout suggested a couple of weeks ago in Edit atom legend in wysiwyg mode:

  return "
  <!--copyright={$atom->sid}--><div class='meta' contentEditable='true'>
    {$atom->rendered->title}, {$by}
  </div><!--END copyright={$atom->sid}-->
  ";

This makes easier for ckeditor to set caret position in the DOM (a single div is possible);

Also, everything with atoms work better if CKEditor “Enter mode” is set to "div" (instead of "p" or "br") but this needs educated contributors...

A-snowboard’s picture

I subscribe to this post because I am very interested.

In a few days I have time to test your patch.

gifad’s picture

Status: Needs work » Needs review
FileSize
4.07 KB

Bugs that need to be fixed:
- Can not insert into a textfield.
- Do not use HTML markup, use SAS instead when insert into textarea/textfield.

Fixed.

BTW, I use $('textarea, .form-type-textfield input') to select textareas and textfields.

jcisio’s picture

Status: Needs review » Needs work

The last submitted patch, scald-use-double-click-in-addition-to-dnd-2073413-13.patch, failed testing.

jcisio’s picture

Status: Needs work » Needs review
FileSize
3.54 KB

Reroll 13.

jcisio’s picture

Issue summary: View changes
Issue tags: +UX improvement
slybud’s picture

Assigned: Unassigned » jcisio

Ok I've thoroughly tested this patch against this configuration

  • drupal 7.23
  • latest scald dev version (doesn't work with 1.1)
  • qtip
  • scald_image + scald_dailymotion + scald_soundcloud

Everything works fine and as described for the following use cases :

  • One content type with single CKEditor field dnd enabled instance
  • One content type with multiple CKEditor field dnd enabled instances (retains cursor position)
  • One content type with single plain text dnd enabled instance (either plain text or filtered text with plain text format) : inserts sas markup
  • One content type with multiple plain text dnd enabled instances (either plain text or filtered text with plain text format)

The only use case not working is when a content type mixes plain text and ckeditor field instances (in thaa case tou can not retain focus in simple textareas, media always get inserted into ckeditor fields)=> don't think this is a use case to consider, so I would be in favor of passing this patch to RTBC IMHO !

jcisio’s picture

Assigned: jcisio » Unassigned
Status: Needs review » Needs work

Nice to see that it works almost everywhere. Now just need to fix the last reported bug. I think in the "mixed case", we should keep trace of the type of the last focus element.

DeFr’s picture

Assigned: Unassigned » slybud
Status: Needs work » Needs review
FileSize
3.58 KB

Here's a re-roll that should handle mix-mode.

slybud’s picture

Assigned: slybud » jcisio
Status: Needs review » Reviewed & tested by the community

Ok great news guyz :

  • patch in #20 is working in any use case (thx DeFr)
  • and it's working on IE8 !!! (which means now that peopole on IE8 can use the Library by doubleclickin' or hitting the "insert" link

Great job everyone and moving this to RTBC !

jcisio’s picture

Assigned: jcisio » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks all! Committed fd91aef and pushed. It solves multiple problems at one: IE8 compatibility, insert an atom inside another atom.

Status: Fixed » Closed (fixed)

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