Add support for Skinr style previews

I agree, and we're doing this in Fusion now (screenshot) with some jquery and span tags in the skin labels. Unfortunately, it's a big PITA when you're dealing with a base/subtheme to specify styles because you don't know what the full path is from the subtheme->base theme. Thus I would really love to see this feature in Skinr itself, where it can handle these kinds of issues.

I would vote for screenshots simply because you want to show an "ideal" version of the skin. Whatever content the user has got on their site may not be ideal for the styling the skin provides... if someone's added a list style to an unformatted view, for example. So screenshots would be helpful in-context documentation about what the style *should* look like, which can help with troubleshooting.

Yes, definitely screenshots. No way I want to deal with actual code for this :)

This would be perfect for 2 places...

1. As a tooltip preview, like TNT does with Fusion
2. As part of an admin listing page #691306: Add an admin page that lists all the places (blocks, content types, views, panels, etc.) Skinr is used

As for implementation, I would follow core's lead:

skinr[style][screenshot] = screenshot-file.png

Jacine, I like that simple implementation approach. Would that assume a certain directory in the theme, or that would be provided as a prefix?

For the different options, I guess it would be set as follows:
skinr[style][options][1][screenshot] = screenshot-file.png

How about some views like preview feature?

We're working on the screenshot previews for this, with the help of mikey_p.

Here are the specs, comments welcome:

• the skin source's info file (skins could come from a combination of the base theme, subtheme, and sites/*/skins) specifies a path relative to itself forskin screenshots with a declaration like skinr[options][screenshots] = skins/screenshots
• not sure what the default should be -- if no dir specified, just use the theme root dir or some other subdir?
• image filenames would be auto-detected based on the skin system name + ID# (optional)
  • so a style defined as: skinr[menu_style][options][1][class] = my-menu-style would have a filename of menu_style-1.png
  • you could also use just menu_style.png, which would be ideal when using the select list as the form element for the styles
  • these should detect a png, jpg, or gif extension
• the files would need to be detected in the order of base theme -> subtheme(s) -> sites/*/skins, so you could override a screenshot in the base theme by placing one with the same filename in the subtheme, for example
• for any screenshots found, skinr would then append an icon to the style when displayed in the UI
• icon = magnifying glass from new D7 set? http://blog.softfacade.com/post/385609105/we-designed-icon-set-for-drupal-7
• on hover, this loads the image in a hover preview with the skin title below
• I'll provide the CSS & graphics for this and any assistance needed with that portion of it

Stephanie, thanks for moving this along. Here are some questions/comments:

1. Do we need to specify an override-able folder for where it searches for screenshots?
2. We could use 'screenshots' or the root of the theme|base_theme|skin as the default location for auto-detection.
3. I think your naming scheme looks good.
4. Detection order looks good.
5. When adding the icon, let's make sure we also have a good non-js fallback here so people can still see the previews if.
6. The D7 magnifying icon looks good. We already borrowed D7's cog from its contextual module.
7. Let's make sure the hover actually displays within the confines of the page (or overlay). This wasn't always the case with the Fusion implementation, if I remember correctly.
8. When we loop through all .info files (of themes and separate skins) we should add the automatically detected filename to the array we create in function _skinr_skinset() as skinr[menu_style][options][1][screenshot] = full_path_from_drupal_root/screenshots/menu-style--1.png. (let's use the D7 [http://drupal.org/node/224333#theme_hook_suggestions_1] convention here of double underscore as separator, where all underscores get converted to dashes in filenames)

Are 1+2 referencing the same thing here? I'm not positive what you mean -- is this is a question should we even have the base path option in the .info file, or have a default dir, or neither?

5 -- yep, definitely, will have it link.

7 -- agreed, we knew this was probably going to be overhauled so didn't bother addressing that issue in Fusion yet.

Thanks!

I guess 1+2 are referencing the same thing, and yes: do we want a default set of locations (in order of priority: theme|base_theme|skin), or require a skinr[options][screenshots] = skins/screenshots, or both?

I personally think the latter is excessive when we have the former.

There are two different things here: one is the precedence of screenshots across different skin sources, the other is establishing the base path *within that source* for screenshots. I think the first one is settled (i.e. it's automated), unless you have comments on the order proposed.

The second one is the question about whether there should be a default dir within the skin's source dir for screenshots to live, like /screenshots within the theme dir. I'm fine with picking a default dir, just wasn't sure what it would be. skins/screenshots is fine by me. Do you think it's ok to mandate that as the only folder where screenshots can live (which is picked up automatically), or should we allow for themers to specify a subdir with the name of their choosing? Themers can be a picky bunch about their naming, but I'm not sure if it's worth having another attribute in the .info file.

Hey guys,

So, I've been talking about this one with Bala, going over ways this could be accomplished, and basically the auto-detection for directory and skin name would add a lot of overhead that none of us will want. The issue basically comes down to performance. We would have to do LOTS of file_exists() checks, and that is expensive and generally undesirable.

Even though it's a little more work for the person defining the .info file, I think the best route here is to specify the full path and the file name (just as Drupal does with all the other .info CSS/JS files) like so:

skinr[skin_name][options][1][screenshot] = screenshots/whatever.png or
skinr[skin_name][options][1][screenshot] = whatever.png

This way you can name your directories and files whatever you want, which is consistent with other Drupalisms, and it will not have a negative performance impact.

Ok, will check in with mikey_p about this, hopefully he hasn't started. Thanks for the feedback. Certainly would make the feature simpler from the point of view of implementing in Skinr, if not for the themer.

We'd still need to have a automatic order of precedence so that subthemes can override base themes' screenshots, so that means they'd have to redefine the parent theme's skin_name screenshot specification.

Linking to this related issue: #733058: Live Preview of Skinr Styles

Ok, I'm marking #733058: Live Preview of Skinr Styles as duplicate and bringing the topic here to continue this discussion.

What I propose is a live/dynamic preview of skinr styles. This may provide the most accurate and efficient previewing method possible for Skinr.

This proposal of live preview was conceptualized mainly in context of editing a skin via the modal skin configuration as demonstrated in my screenshot.

Choosing a style from the drop-down menu in a Skinr modal configuration box would trigger an instantaneous live preview on the actual element being skin'd, without the necessity to save or close this the dialog box.

How? The selection of a style/option from the drop-down menu would call a JS function that would then insert the appropriate classes into the HTML markup of the element being skin'd. This would be temporary and for previewing purposes only, as Skinr handles adding the classes after settings are saved in a completely different way.

The fact that the modal Skinr configuration dialog boxes are already drag-droppable is a great thing and means you can easily move them away from the element being skin'd so that your live preview is unobstructed.

I believe this feature could take Skinr to a whole new level in terms of UX, but implementation could prove much more challenging than imagined.

Jonathan, personally I would prefer to see that left in a separate issue, as this one will hopefully be marked fixed in a few days when the screenshot preview functionality is complete, but live previews are another set of functionality.

While live previews would be a cool feature, they don't take the place of screenshots anyway, since many Skinr styles are based on content which may not yet be present -- and the theme should be able to give an accurate preview of the style under ideal conditions.

True... Marked the other issue active again.

Makes sense, Steph!

Nice to hear that the Screenshot functionality may be fully ready within a few days. Screenshot previews is definitely a nice feature and I don't think live previews would replace it. Especially considering live previews would only work in the context of modal Skinr Configuration box, and the other reasons you mentioned.

How about displaying the preview in an iframe or through some ajax magic? It may be simpler, as screenshots are not the most straightforward thing with php :)

Think of the Views preview functionality.

subscribing

Here's an initial patch that covers that php side.

I'm not sure what other elements than options should be able to support previews. It seems that there should be a better way to do this, but it looks like by the time that base theme skins are merged with the current theme in _skinr_skinset() that there is no longer anyway to tell what theme/module the screenshot was defined in.

You can also get a pretty view of this patch at: http://github.com/mikeyp/skinr/compare/DRUPAL-6--2...previews

Forgot patch

This remove the link and makes the whole image inline (but hidden by default*). This should make it easy to pretty much whatever the simplest thing is, with css, or js to display the preview. Also for super flexibility, the preview is generated with a theme function so it could be overridden for different functionality.

I'm open to suggestions for what the default behavior should be. Slide toggle maybe?

* This probably shouldn't be inline as that makes it too hard to override with a theme.

I just looked through the patch. There are a few obvious issues that I found:

• code won't work with PHP4 due to using references in foreach loops
• the function skinr_ui_info_options_to_form_options() is used for drop-downs (select) as well as the checkboxes and radio buttons. That means we're likely to get html in the select list if, the way the code is currently written

I need to test this, and add some basic effect for toggling the preview.

Thanks mikey_p!

Steph/Chris, have you guys tested this? I don't have the time to set this up properly to test. I assume you have the code/screenshots you will be using for this available somewhere? Any chance you can share so we can give this a try?

We would like to wrap this up and do a release shortly.

Jacine,

Okay, I'll be taking a look at the patch.

mikey_p,

Your patch won't apply against the latest version of Skinr 6.x-2.x-dev (2010-Mar-28). Could you roll a new patch against the latest version?

Thanks!

Certainly

Jacine,

I've been testing mikey_p's patch for screenshots and chatting with him about an image path issue that came up.

But there still needs to be a display method. As mentioned above, the patch inserts an inline img tag for the screenshot and sets display:none on it.

mikey_p says he'd be able to implement the display based on a mockup of how we'd like to do it.

Jacine, since I'm guessing you have some idea of how you'd like the screenshot to display, could you provide mikey_p with some direction?

Meanwhile, I'll continue to test the patch for functionality.

Reroll

I just tried the patch and it's not working for me. This could be because I am using a subtheme, but I don't think so... I was expecting the path to be broken, but I'm not even seeing ['screenshot'] set anywhere. There are also a few places I saw, like in _skinr_skinset() where it seems that screenshot should be listed and it's not.

I don't like how this is being sort of hacked into form labels in skinr_ui_info_options_to_form_options(). I don't think it belongs there. The labels need to be translatable, which wont happen with this markup in there, and really that is just a helper function to just get the options in the right format for the widgets. #suffix should be used for this instead.

Since I can't get this working, this is what I would like to see as far as markup goes:

<a href="#" class="skinr-ui-toggle-preview">Preview</a>
<div class="skinr-ui-preview" id="skinr-ui-preview-[skin-name]">
  <ul>
  <li class="skinr-ui-thumbnail skinr-ui-thumbnail-[skin-name]-[option-number]">
    <p>[option-label]</p>
    <img src="whatever-1.jpg" alt="[option-label]" />
  </li>
  <li class="skinr-ui-thumbnail skinr-ui-thumbnail-[skin-name]-[option-number]">
    <p>[option-label]</p>
    <img src="whatever-2.jpg" alt="[option-label]" />
  </ul>
</div>

No, display:none inline... It needs to be style-able with or without JavaScript. I'm not a jQuery expert or anything, but I was thinking something along these lines would be good:

1. .js .skinr-ui-preview should display: none by default
2. The .skinr-ui-toggle-preview link should act as the trigger on click to show the closest .skinr-ui-preview. It should use behaviors so themes can override this if they choose to.
3. The jQuery should add a helper class to the wrapper div when it's showing. .skinr-ui-preview-activated or something
4. It should display: none again when clicking on the toggle link, or when clicking outside of it

Oh, and also let's call this [thumbnail] instead of [screenshot].

Just to make this clear you want to show all the previews in a list after the form element? I believe that removes some of the original intent of this patch, that previews would be displayed next to the actual options, so that when hovering over, or clicking on an item, you would see a preview of just that option.

Just wondering if anything is going on with this?
I assume this was before live previews were implemented... is this really needed?

Closing D6 tickets.