Make Galleria more themeable [#458448]

Much of Galleria's page markup is currently hidden away in galleria.module, but I think these elements are better off in galleria.tpl.php. That way, themers have greater control over Galleria's appearance when overriding the template.

I've attached a patch with the following changes:

The main-image div is now hard-coded into the Galleria template.
The next and previous links are now separate template variables.
Added "galleria-next" and "galleria-previous" classes to the next/previous links, respectively.
Added an $image_count template variable to control single-image behavior. This can utilized to make advanced behavior changes based on the number of uploaded images.
Admin page:
- Removed the thumbnails-location setting (the thumbnails, main image, and next/previous links can all be rearranged in the Galleria template).
- Added fields to control the contents of the next/previous links. This is useful, for example, if you want to display images for these links.

Comment	File	Size	Author
#4	galleria_themeable_4.patch	5.04 KB	eromba
#2	galleria_themeable_3.patch	5.03 KB	eromba
#1	galleria-themeable-458448-1.patch	5.21 KB	Mark Theunissen
	galleria_themeable.patch	4.77 KB	eromba

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Mark Theunissen CreditAttribution: Mark Theunissen commented 14 May 2009 at 09:22

File	Size
galleria-themeable-458448-1.patch	5.21 KB

This is a great idea!

The main issue I have with the patch in it's current form is the security risk of allowing anyone with "Administer Galleria" permission to insert HTML into the page by using the "next" / "prev" link field. There's been a bit of buzz lately on the development list about these sorts of risks, I'd rather avoid it if possible.

I've modified the patch slightly. In the TPL file, I've put the html for the next/prev links, with the words "next" and "previous" being passed through in template variables so that they can still be translated.

I've also removed the inline Javascript that hides the images before galleria has run. In my testing, I've found that putting the following CSS in galleria.css works just as well and is far less confusing:

ul.gallery img {
  display: none;
}

Is this right? Is there any reason why you chose inline JS over CSS?

But otherwise everything else is still in. Please review new patch before I commit.

Comment #2

eromba CreditAttribution: eromba commented 14 May 2009 at 21:38

Status:

Needs review

» Reviewed & tested by the community

File	Size
galleria_themeable_3.patch	5.03 KB

Good thinking about the security risk. Also, moving all of the next/previous-link markup to the TPL makes the module even more themeable.

As far as the hiding behavior, we do want to keep the JavaScript, because going with straight CSS means we're relying on Galleria to unhide images all the time. This is fine for most users, except those who have JavaScript disabled (they won't see any images whatsoever). By hiding the images via JavaScript, we ensure that the user has JavaScript enabled and that the images will eventually be unhidden.

I've attached a new patch identical to yours, minus the switch to CSS.

And with that, I'm out of ideas for this module! ;) I think it's long overdue for another beta version one of these days. How about it?

Thanks for all of your work!

Comment #3

Mark Theunissen CreditAttribution: Mark Theunissen commented 15 May 2009 at 11:23

Yes, you're right, I wasn't thinking about disabled JS....

Hmmm, I still have a hunch that putting the JS in the template file is not the "drupal" way ... what about doing the following call in galleria_includes()...

    drupal_add_js("$('ul.gallery img').css('display', 'none');", 'inline', 'footer');

Do you know of any examples elsewhere in contrib where a similar approach is taken? What are the best practices? I think that the above call is correct, but maybe it doesn't work as well?

I'm keen to release a 1.0 version as soon as the last patches are tied up!

Comment #4

eromba CreditAttribution: eromba commented 16 May 2009 at 05:03

File	Size
galleria_themeable_4.patch	5.04 KB

The reason why I put the JS command directly in the template is to ensure that the images are hidden as soon as their markup is loaded into the DOM. Using drupal_add_js results in a delay (regardless of whether the code is inserted into the header or the footer) and the images are not hidden until the page loads completely, resulting in the same flicker that this JS command was intended to prevent in the first place.

As far as best practices go for this sort of thing, Drupal naturally encourages the use of drupal_add_js() whenever possible, but in this case the intended functionality can only be achieved by placing the command in a specific place in the template and as such is permissible there. Furthermore, we have the code wrapped in CDATA tags to comply with XHTML standards, so I believe we have all of our bases covered.

Speaking of XHTML compliance, it looks like the "<<" and ">>" characters in the next/previous links cause W3C validation errors when they are in plain text form. Attached is the patch from #2 with these characters replaced by the appropriate HTML entities.