Click handlers for image and album thumbnails.

[setnes]

This is the start of #466. This is not yet complete. The navigation works using click handlers, but the anchor tags still exist... just without any href. This allows review of the click logic code without needing to refactor the anchor tag logic and any special CSS references... yet.

Also... using replaceState isn't what we needed here. We actually do want the URL in the history and a page navigation to occur (not a reload). A simple location.href assignment makes things work very much like the old links. The labels are no longer in the way of clicking.

Do we want to test this against the gradient label changes in #456 before refactoring further?

[oparoz]

Thanks!

Testing #456 would be great. It should simply be a matter of merging the CSS changes.

[setnes]

I merged the changes from #456 into this. It seems to work.

I think the album label needs to be somewhat larger... or at least somehow different than the image label. A folder with just one picture is a good test case for this. Also, should there be CSS to change the cursor to "pointer" when hovering over an image or album?

The anchor tags still need to be refactored. Right now they are still just placeholders without any href defined so we can prove it all works.

[oparoz]

Looking at the commits, it seems you took everything in #456, but we only need the CSS. It's important (for the test) because the element swapping is what is messing future plans up.

I think the album label needs to be somewhat larger.

I did try to use something different, but couldn't find a satisfactory solution, mostly because we can't use bold and because albums and pictures come in many different sizes.

Also, should there be CSS to change the cursor to "pointer" when hovering over an image or album?

It's like that on master, so it has to be the same in this PR. I haven't checked where it's missing.

[setnes]

We need more than the CSS to make the change work. For example, the CSS is referencing the title class under the album-label class. I needed to merge some of the changes into my galleryalbum.js and galleryimage.js or it would not work. No worries.

Adding a "cursor: pointer;" to the CSS is all that is needed to fix the cursor. The cursor changes in master only because of the anchor tags.

[setnes]

I have refactored the anchor tags. Everything seems to be working with the exception of keyboard navigation. Give it a try.

[oparoz]

Acceptance tests

• Clicking on album opens the album
• Clicking on album label opens the album
• Clicking on an album shows a spinner
• Clicking on image launches the slideshow
• Clicking on image label launches the slideshow
• If an album contains a single bad JPEG, the media type icon is shown
• Clicking on an album containing a single bad JPEG opens the album. For testing purposes, please only put a couple of pictures in the album
• Clicking on that album shows a spinner
• If an album contains a bad JPEG, that image is removed from the representation
• Clicking on an album containing a bad JPEG opens the album, as usual
• Clicking on that album shows a spinner
• Future plans aren't ruined (Internal)

[setnes]

Should we really be doing this? We can make it work with an anchor tag.

This is pretty insightful. http://stackoverflow.com/a/32194060

[oparoz]

Thanks for digging that out. I only looked at performance comparisons when trying to decide if switching was wise, but this seems like a valid point, especially for accessibility reasons.
So maybe we should wrap each div in an anchor.

[setnes]

Yeah. I think we can have both the anchor tag and a click handler. I want a click handler on the image anyway so I can set a variable to fix the issue with closing the slideshow when there is no history. We already have a need for the click handler on the album to show the spinner.

[oparoz]

Cool. I hope it won't be a headache with getting the size of elements and resizing them. We'll see :)

[setnes]

The latest commit is working with the anchor tags and label gradients.
I lost evidence that @raghunayyar made the CSS changes when I rebased my branch. I think this all needs to be merged together though. Should I try to fix that?

[oparoz]

In your commit, you wrapped the label with the anchor, but we need to wrap the container or we're back to square one.

Something like this in galleryimage.js:

    <a href="{{targetPath}}">
        '<div class="item-container image-container" ' +
        'style="width: {{targetWidth}}px; height: {{targetHeight}}px;" ' +
        'data-width="{{targetWidth}}" data-height="{{targetHeight}}">' +
        '   <span class="image-label">' +
        '       <span class="title">{{label}}</span>' +
        '   </span>' +
        '   <span class="image" data-path="{{path}}"></span>' +
        '</div>' +
    '</a>';

Hopefully this doesn't introduce any new issues.

[setnes]

I did mess with that, but I was having problems having the album-loading-spinner-thingy show up correctly if it was nested inside the anchor tag. The code in my branch is working very well for me. Do you have future plans where getting rid of the parent div tag is important?

[setnes]

I've gone through all of the tests except for "future plans" one. :)

[oparoz]

Thanks!

The one plan where there is a conflict is the "instant thumbnails" PR. With your changes, the labels are resized with the images and we don't want that.

[setnes]

I made the anchor the outside element. It now has the container classes as the div was suddenly redundant. I hope this helps.

This is passing the same tests as before. Also, we should think about removing the 1px margin between the cropped images in the album link. The label shows better that way. (not in this change)

Make up your own snarky comment about how this pull came first. 😏

[oparoz]

Closing as this has been added to the 9.1-next branch: a3a3a50
Thank you for your contribution!