It's one of the planned features, right?

CommentFileSizeAuthor
#8 add_block-1220480-8.patch3.79 KBfirebus
#8 add_block-1220480-8-D6.patch3.95 KBfirebus
#6 add_block-1220480-6.patch3.92 KBfirebus
#1 add_block-1220480-1.patch3.89 KBfirebus

Comments

firebus’s picture

firebus commented
StatusFileSize
new add_block-1220480-1.patch3.89 KB

Here's a patch that creates a block.

I'm not very familiar with the way these chat links are built, so it's possible that this implementation is missing a *lot* of the configuration required to provide a generally useful block. If anyone knows what's missing, please let me know! Pointers to good documentation about how to build chat links would also be welcome.

The block provides a link that contains an image. If there's a live_person directory in the active theme, the block will assume it contains reponline, repoffline, and repoccupied images and will include the imageUrl parameter in the source of the image. If not, the imageUrl parameter is omitted and Live Person default images are used.

I intend to add a pop-up feature shortly, which will open the chat in a small pop-up window onclick. There is block configuration for width and height that will be used by the pop-up feature.

firebus’s picture

firebus commented
Status: Active » Needs review
nicholas.alipaz’s picture

nicholas.alipaz commented
Status: Needs review » Needs work

Thanks for the patch. I currently don't have anymore clients using live person so I can't test really. However, the patch still needs a little work.
<img src=.... part should likely be replaced with theme('image', ....);

$_SERVER['SERVER_NAME'] should probably use global $base_url.

That is all I see at a glance without testing.

firebus’s picture

firebus commented

thanks for your feedback! i'll update the patch wrt theme('image').

i'm pretty sure base_url will not work because we need to provide an absolute URL to live person in the imageUrl parameter, but i'll try it again to make sure.

nicholas.alipaz’s picture

nicholas.alipaz commented

You could use url() to output the link and set 'absolute' => TRUE for the full url.

firebus’s picture

firebus commented
Status: Needs work » Needs review
StatusFileSize
new add_block-1220480-6.patch3.92 KB

you were right the first time - $base_url has a fully qualified absolute URL. i was confusing with base_path() (which i'd tried earlier) which only provides a relative URL.

here's a reroll with $base_url and a call to theme('image',...). this is also a git patch against 6.x-1.x-dev, so a little different than the previous.

please let me know how/if you'd like to move forward with this - i have a client that would like to use this module, and it would be great to get this change (and the forthcoming pop-up window feature) into a 1.2 release...

nicholas.alipaz’s picture

nicholas.alipaz commented

Actually I think url is probably better http://api.drupal.org/api/drupal/includes--common.inc/function/url/6

But I can change before committing. Not sure if there is anything else you are trying to get into this patch. Like I said, I have no real way of testing anymore. But I will be glad to roll it into dev and push it to a release with the other feature. I wouldn't expect a block of this sort to break anything really.

firebus’s picture

firebus commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
StatusFileSize
new add_block-1220480-8-D6.patch3.95 KB
new add_block-1220480-8.patch3.79 KB

I really appreciate your willingness to work on getting this committed! I also don't think that these changes should affect the existing functionality, and haven't found any problems in testing.

You're right that url() is better, I wasn't aware of the absolute option.

Here's a reroll using url(), and a patch that works on D7. Hopefully it will pass automated tests.

There's nothing else I want to add to this patch, but I'll open a separate feature issue for the pop-up window on Monday with a patch.

nicholas.alipaz’s picture

nicholas.alipaz commented
Status: Needs review » Postponed (maintainer needs more info)

I was looking over this patch and readying committing it to the repo. I noticed this part:

/**
 * Check for a live_person directory in the theme and use that directory for
 * images in the block.
 *
 * @return mixed
 *  A URL string or FALSE if there is no directory in the theme
 */
function _live_person_get_image_url() {
  global $theme;
  $live_person_theme_path = drupal_get_path('theme', $theme) . '/live_person';
  if (file_exists($live_person_theme_path)) {
    global $base_url;
    return url($base_url . '/'. $live_person_theme_path, array('absolute' => TRUE));
  }
  else {
    return FALSE;
  }
}

What exactly is going on here? I assume this is allowing configurable images, but there is no patch to add hook_help(), a README.txt, or possibly a handbook page to explain this. If you are indeed allowing configurable images, it would be IMO a better option to allow uploading the images from the block configuration and/or specifying their path.

firebus’s picture

firebus commented

By default, Live Person will provide stock images for the offline, busy, and open states.

However, you can pass an ImageUrl parameter through, and Live Person will use that as a path for these images. It expects to find images named repoffline.gif, reponline.gif and repoccupied.gif in the directory.

This patch is looking for a directory named "live_person" in the active theme, and is supplying that as the ImageUrl parameter if it exists. This seemed like the easiest way to provide the feature.

Since there's no existing README.txt, help hooks, or documentation node for this module, I had nothing to add documentation to. Let's open a new documentation feature issue to handle writing documentation for the module, which should be a quick task whichever method is preferable.

If you'd prefer to have this handled through block configuration, let me know if you think file upload or file paths would be better. Like you, I have a client that needs this functionality and I'm attempting to do the minimum amount of work to meet my use case, while providing something useful that others can build on. I'm happy to do more (within reason) if you think it's necessary to commit the feature, but I'd like to avoid striving for perfection!

nicholas.alipaz’s picture

nicholas.alipaz commented
Status: Postponed (maintainer needs more info) » Needs review

I went ahead and committed changes to both 6.x and 7.x:
http://drupal.org/commitlog/commit/12642/7e652d6ea2f79282972aa19ec54fc9e...
http://drupal.org/commitlog/commit/12642/19264011b2437e66dc575b985b55f07...

Please test the copy in the git repo and let me know that all is well. I can then create a new release.

firebus’s picture

firebus commented
Status: Needs review » Fixed

Looks good to me.

I tested both D6 and D7 on a clean install of both Drupal and live_person and confirmed that:

- the live person javascript is being inserted correctly
- the block works along with its image override feature
- the popup javascript works

Thank you!

nicholas.alipaz’s picture

nicholas.alipaz commented

Thanks for the patches and work. New releases have now been created and should be available once the packaging script runs.

Status: Fixed » Closed (fixed)

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