Hi Manuel!

Found a bug in galleryformatter.js. Have a look at the following lines of code.

  var $locationHash = window.location.hash; // if we are being deeplinked to a specific slide, capture that

  // if we have a hash in the url
  if ($locationHash) {
   $slides.filter($locationHash).show(); // show that slide
   $thumbsLi.not($(".cloned")).find("a[href="+ $locationHash +"]").parent().addClass('active'); // activate that thumbnail
  }
  // otherwise the default
  else {
    $slides.filter(':first').show(); // show the first one
    $thumbsLi.filter('.slide-0:not("cloned")').addClass('active'); // activate the first thumbnail
  }

Now we have a node with the image gallery and comments enabled, imagine a link like /node/1234#comment-1. Maybe you already know what happens. The browser will jump directly to the comment but if the user now scrolls to the image gallery no image is selected. Because we never check if the anchor from the URI is really a image slide.

My solution:

  var $locationHash = window.location.hash; // if we are being deeplinked to a specific slide, capture that

  // if we have a hash in the url
  if ($locationHash && $thumbsLi.not($(".cloned")).find("a[href="+ $locationHash +"]").length !== 0) {
   $slides.filter($locationHash).show(); // show that slide
   $thumbsLi.not($(".cloned")).find("a[href="+ $locationHash +"]").parent().addClass('active'); // activate that thumbnail
  }
  // otherwise the default
  else {
    $slides.filter(':first').show(); // show the first one
    $thumbsLi.filter('.slide-0:not("cloned")').addClass('active'); // activate the first thumbnail
  }

By the way, at the end of the file a semicolon is missing and the variable $slideContainer is never used within the script.

CommentFileSizeAuthor
#6 galleryformatter-858664-2.patch1.63 KBmanuel garcia
#4 galleryformatter-858664.patch1.58 KBmanuel garcia

Comments

manuel garcia’s picture

manuel garcia commented
Assigned: Unassigned » manuel garcia
Status: Active » Needs work

Good catch Fleshgrinder!

Your solution is the way to go, only we should probably do that selection once and cache it, since we use it again later. I'll get to it when i get a minute.

I'll also take a look at what you mention in the end - Thanks!

fleshgrinder’s picture

fleshgrinder commented

Make use of JSLint to find such little “errors” like the two things I mentioned at last.

manuel garcia’s picture

manuel garcia commented

Yep, that's a nice tool - I've made a commit that fixes these little "errors".

More on the real bug tomorrow.

manuel garcia’s picture

manuel garcia commented
Status: Needs work » Needs review
StatusFileSize
new galleryformatter-858664.patch1.58 KB

OK, here is my take on the problem.

I check wether we have a slide to show based on that hash first, if so we show it, otherwise we show the first slide.

In order to avoid duplicate code, I created a helper function called showFirstSlide();

Please test attached patch and let me know how it goes!

fleshgrinder’s picture

fleshgrinder commented

.size() is slower then .find(), see this comment on jQuery API.

If there are more elements found then exactly one, it's the problem of the browser to which element it will set the focus, not our's.

EDIT: My full script I'm using at the moment (with your patch incorporated):

// $Id: galleryformatter.js,v 1.1.2.5 2010/07/18 22:39:54 manuelgarcia Exp $

Drupal.behaviors.galleryformatter = function(context) {
  $('.galleryformatter:not(.gallery-processed)', context).each(function() {
    Drupal.galleryformatter.prepare(this);
  }).addClass('gallery-processed');
};

Drupal.galleryformatter = Drupal.galleryformatter || {};

// setting up the main behaviour
Drupal.galleryformatter.prepare = function(el) {
  var $el = $(el),
      $slides = $('li.gallery-slide', $el),
      $thumbs = $('.gallery-thumbs', $el),
      $thumbsLi = $('li', $thumbs),
      thumbWidth = $thumbsLi.filter(':first').width() +'px',
      liWidth = $thumbsLi.outerWidth(), // includes padding
      $locationHash = window.location.hash; // if we are being deeplinked to a specific slide, capture that

  /*
   * Only start the thumbs carrousel if needed
   */
  if (($thumbsLi.size() * liWidth) > $thumbs.width()) {
    $('ul', $thumbs).width('9999px');
    $thumbs.infiniteCarousel();
    $thumbsLi = $('li', $thumbs); // we need to reselect because infiniteCarousel inserts new empty li elements if necessary
  }

  $thumbsLi.each(function() {
    $(this).css({width: thumbWidth});
  });
  var $thumbslinks = $('a', $thumbsLi);

  /*
   * @TODO:
   * figure out how to get this into proper functions reusing selections
   */
  $thumbslinks.click(function(e) {
    $hash = $(this.hash);

    if (!$hash.is(':visible')) {
      $thumbsLi.removeClass('active');
      $(this).parent().addClass('active');
      $slides.filter(':visible').fadeOut('slow');
      $hash.fadeIn('slow');
      /*
       * @FIXME
       * Need to figure out a way to update the location bar of the browser, for bookmarking etc, without making the scroll jump
       * window.location.hash = this.hash; solution below does update the location, but makes the scroll jump.
       */
      // window.location.hash = this.hash;  // not sure if this is the best way to do it.
    }
    e.preventDefault();
  });

  /*
   *  Startup behaviour (when the page first loads)
   */
  $slides.hide(); // hide all slides

  // Select first slide to display.
  function showFirstSlide() {
    $slides.filter(':first').show(); // show the first one
    $thumbsLi.filter('.slide-0:not("cloned")').addClass('active'); // activate the first thumbnail
  }

  // if we have a hash in the url
  if ($locationHash) {
    var $slideToShow = $slides.filter($locationHash); // show that slide
    if ($slideToShow.find() !== 0) {
      $slideToShow.show(); // show that slide
      $thumbsLi.not($('.cloned')).find('a[href='+ $locationHash +']').parent().addClass('active'); // activate that thumbnail
    }
  }
  // otherwise the default
  else {
    showFirstSlide();
  }
};

It's working perfectly, you have the link to my current project and you can see it there in action.

manuel garcia’s picture

manuel garcia commented
StatusFileSize
new galleryformatter-858664-2.patch1.63 KB

OK, well, I think it actualy makes more sense to just use the length property directly, since thats all that size is returning anyway. It's a minimal difference but oh well.

I've updated the patch with a little more commenting. BTW, I dont think it makes sense to call find() there, the $slideToShow already has length in it that tells u how many elements it contains.

I've also changed the if statement for > 0.

manuel garcia’s picture

manuel garcia commented
Status: Needs review » Fixed

committed - thanks!

fleshgrinder’s picture

fleshgrinder commented

.length is perfect, no function call at all! I think the second patch is great. :-)

Status: Fixed » Closed (fixed)

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