We wanted to make our popups look like Facebook on our site and be able to just turn on a switch to do so, so I came up with a way that we can skin these puppies and choose a skin within the admin interface:
Popups Settings

The Basic skin:
Popups Settings

The Facebook skin:
Popups Settings

Of course we can change some of the default Facebook skin before potentially committing this to the project.

The technique I'm using is a simple module_invoke_all('popups_skins') so that other modules can provide their own popups skins easily. Then I'm using a hook_theme_registry_alter() to apply a new popups-popup.tpl.php file located within the popups/skins/SKIN-NAME folder. Also css and optionally an additional js file (used to override Drupal.theme.popupLoading and/or but additional Drupal.behaviors within) can be provided with your skin inside of your skin folder. These are then added conditionally within popups_add_popups().

I also moved the default popups-popup.tpl and popups-skin.css into the popups/skins/basic folder and this is the default skin if nothing one isn't chosen.

Check out the diff and attached files.

CommentFileSizeAuthor
#4 372378.patch11.83 KBRobLoach
skins.zip10.24 KBsirkitree
popups_skins.patch4 KBsirkitree
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

davidburns’s picture

looks great, very nice work.

RobLoach’s picture

Haha, nicely done, Jerad. Would be great to extend popup discovery mechanism to the theme's /popups folder, or something...... sites/all/themes/zen/popups, kinda thing?

starbow’s picture

Status: Active » Needs review

This looks like fun. I will definitely give it a try.

RobLoach’s picture

Version: 6.x-2.x-dev » 6.x-1.x-dev
FileSize
11.83 KB

Might be good to cache the skins, popups_popups_skins() is pretty expensive. This patch adds the popups skins registry (through function popups_skins).

Note that this patch doesn't include the Facebook skin images. Just got an idea: #373754: jQuery UI Dialog Popup Skin

sumitk’s picture

>>>>subscribing<<<<

starbow’s picture

Status: Needs review » Needs work

Good start. This is definitely going in.
* The "Close" in facebook popup needs more contrast.
* The facebook loading does not seem to show up. The code is getting run, but it isn't visible...not sure why but I tested on firefox 3 and ie7.
* Is facebock going to come after us for stealing there theme?
* Shouldn't popups_skins() be used in popups_theme_registry_alter()

RobLoach’s picture

Got an idea. Instead of implementing popups_theme_registry_alter(), we use 'template' => variable_get('popups_skin', 'popups-basic.tpl.php') in hook_theme(). This would make it so that the theme registry is built with the popup skin template instead of having to use hook_theme_regiistry_alter() to change it.

starbow’s picture

I would love to get folks opinion about moving the popup template out of the tpl.php file and into the popups.js. Then it would be themable just Drupal.theme, just like the loading layer. The downside is it is a little uglier (especially since javascript doesn't have multiline string), and a bit more obscure for themer. The upside is it fixes #361957: Breaks admin/build/block without having to toss the template into a cookie.
And it seems like it would simplify this patch a bit as well.

starbow’s picture

Status: Needs work » Fixed

A version of this has been committed to dev, and will be part of 1.2-alpha2.
Bring on the skins!

RobLoach’s picture

I still see the following:

function popups_theme($existing, $type) {  
  return array(
    'popups_popup' => array(
      'template' => 'popups-popup',
    ),
    'popups_save_dialog' => array(),      
  );
}

Is the popups_popup template used anymore?

starbow’s picture

Good call. That can come out now.

starbow’s picture

Status: Fixed » Needs work

Just a reminder for myself.

starbow’s picture

Status: Needs work » Fixed

Removed from dev

Status: Fixed » Closed (fixed)

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