Hi,

Not real sure where to post this, so if it is in the wrong spot let me know and I'll move it.

Basically I've made a few small changes to the image picker to fix some bugs and functionality.

The attached patch does the following:
- Separates settings from default options. (ie node types are not default settings where as the sizes are).
- Added in default settings for align, show and link options. (ie the insert part of the module).
- Automatically jump the browser back up to the edit field when "Insert Image" is pressed (only for nodes, not comments).
- Fixed a bug in the detection of tinyMCE.

The patch is a little hard to follow, but feel free to pull out the parts you need (I normally use bzr to manage my patches so if it needs redoing or something let me know and I'll help how I can).

Let me know if you have any questions.

Cheers,
Josh

CommentFileSizeAuthor
imagepicker.diff6.14 KBJoshua Hesketh

Comments

hutch’s picture

hutch commented

Thank you for the patch, I cannot get my *nix patch tool to work with it but never mind, it's a small patch so I'll do it by hand and I'll report back when I've got it working.

One question though, does the Tinymce change make it work in IE7? There is an outstanding bug report saying that it does not and I've no Windows machine to test that.

more soon

hutch’s picture

hutch commented

Well I have got the patch in and it is working, the settings page is much improved.
The iframe 'Insert Image' button works better, I don't need to click in the body before hitting the button. ;-)

One more question though, just to be sure, I am not sure I have put the "win.location.hash='body_hash';" line in the right place, here is where I have it now:
from line 573 on in imagepicker.module

      else {
        var nodeBody = win.document.getElementById('edit-body');
        var commentBody = win.document.getElementById('edit-comment');
        if (nodeBody) {
          insertAtCursor(nodeBody, imgpInsertion);
        }
        if (commentBody) {
          insertAtCursor(commentBody, imgpInsertion);
        }
      }
      win.location.hash='body_hash';
    }
  }

Is that correct?
I guess that it is but I would like confirmation before I commit.

I get
node/add/page#body_hash
in the location bar on my browser and the page jumps to it, nice touch, I have been wondering how to do that.

Nice work!

Joshua Hesketh’s picture

Joshua Hesketh commented

Glad you managed to get it working :).

That is the correct place for the hash code, yes. While the method I used in the patch seems to work without any problems there are nicer ways of doing it. This way requires us to put in an anchor link at the body field, modifying the body form. It would be nice if we could use jquery (the default drupal JS library) to jump up to the body div without having to modify the body form but I didn't have time to get this to work. I played around with jquery a little bit but I ran into trouble with the iframe and whatnot. (If you do play with it, you'll have to somehow send to the parent window where to jump to).

Let me know if you have any more questions.

Cheers,
Josh

hutch’s picture

hutch commented
Status: Needs review » Reviewed & tested by the community

Good. I thought is was right but no harm in checking. I will commit this to dev shortly.
I will play around with jquery but as you say, the iframe makes it a bit difficult in this respect.

Thanks for your help!

Joshua Hesketh’s picture

Joshua Hesketh commented

Actually, I was thinking... A better way of doing it would be to give the text field focus (document.getElementById().focus). However determining the field ID for the body or comment area would be very troublesome... (You could still use jquery to scroll to it smoothly). But if all else fails, this basic jump works well enough :)

hutch’s picture

hutch commented

I'm going to leave this as is for now. I've added your changes to drupal 5.x-dev as well.

hutch’s picture

hutch commented
Status: Reviewed & tested by the community » Closed (fixed)

Issue closed as this patch is now in the stable releases for D5 and D6