Skinr will provide a plugin containing groups for skin implementors to utilize when creating skins.

This will hopefully help to provide some consistency in the UI, by promoting the re-use of groups as categories.

CommentFileSizeAuthor
#13 skinr_977110_13.patch799 bytesmoonray

Comments

jacine’s picture

Here's what I'm thinking for this:


/**
 * Implements hook_skinr_group_info().
 */
function skinr_default_skinr_group_info() {
  // Default container for skins that do not define a group.
  $groups['general'] = array(
    'title' => t('General'),
  );
  // Grid, layout and other structural related skins.
  $groups['layout'] = array(
    'title' => t('Layout'),
  );
  // General box (presentational) skins.
  $groups['style'] = array(
    'title' => t('Style'),
  );
  // Fonts, styles, sizes and other typography related skins.
  $groups['typography'] = array(
    'title' => t('Typography'),
  );
  // Advanced options, including manually entered CSS classes.
  $groups['advanced'] = array(
    'title' => t('Advanced'),
  );
  // Developer information, e.g. available theme hooks.
  $groups['information'] = array(
    'title' => t('Information'),
  );
  return $groups;
}

nomonstersinme’s picture

Status: Active » Needs review

no description field? only asking because the version of the skinr.inc that i looked at today had it. otherwise this makes sense to me.

moonray’s picture

Seconding what amanda said.

jacine’s picture

Not if it's going to print on the settings form. This is what I wanted the admin description for, but...

Any other feedback on the actual groups?

jacine’s picture

The comments above each would serve as the description though, FTR.

jacine’s picture

I discussed this with @sociotech and have tweaked it a little.

The "General" group is a bit of a concern, and we've agreed not to use it, and instead let "Style" group be the default. We've also slightly changed the order based on the frequency of use.


/**
 * Implements hook_skinr_group_info().
 */
function skinr_default_skinr_group_info() {
  // General box (presentational) styles; default group for skins
  $group['style'] = array(
    'title' => t('Style'),
  );
  // Fonts, styles, sizes and other typography related skins.
  $group['typography'] = array(
    'title' => t('Typography'),
  );
  // Grid, layout and other structural related skins.
  $groups['layout'] = array(
    'title' => t('Layout'),
  );
  // Advanced options, including manually entered CSS classes.
  $group['advanced'] = array(
    'title' => t('Advanced'),
  );
  // Developer information, e.g. available theme hooks.
  $group['information'] = array(
    'title' => t('Information'),
  );
  return $groups;
}
sun’s picture

Hm. Couple of issues here:

1) "styles" -- all skins are styles; doesn't make much sense for me. Contrary to that, layout and typography make perfectly sense to me. I'd rather go with a "default", "general", or "other" group for all skins that don't belong into layout and typography, and revisit the necessity for further default groups at a later point in time.

2) "advanced" and "information" do not seem to be actual groups for skins -- rather additional groups in the theme hook configuration form...?

jacine’s picture

1) "styles" -- all skins are styles; doesn't make much sense for me

All may technically be styles, but all styles are not equal. Default is definitely not better for this because it doesn't say anything about what type of skin is inside. I don't like General for the same reason, but I know we need something for a default and after discussing with @sociotech I feel good about Styles over General. I realize it's not perfect and I'm open to other names, but the purpose of the Styles group is pretty clear to me and the other people (that write skins) which have provided feedback so far. It's where block, pane and menu styles would go. If you can think of a better name, I'd love to hear it.

2) "advanced" and "information" do not seem to be actual groups for skins -- rather additional groups in the theme hook configuration form...?

I don't know about this. I think advanced would be fine as a group, as there are other non-skins that could go in here eventually, such as template support, but information doesn't really belong here. Both of these buckets do need to go in, but I don't care "how" they get there, so I'm fine taking them both out of here.

ChrisBryant’s picture

This looks good and makes sense to me.

I was thinking about specific examples and thought about if I had a skin that created a dropdown menu or a skin that implemented a Lightbox style effect, what would those be categorized as?

In this case I would put both of those skins under the default "Style" group, but I guess they could create their own groups for "Menu" and "Overlay" if it made the most sense.

Where would you recommend grouping those example skins?

jacine’s picture

Where would you recommend grouping those example skins?

Under Style. I went back and forth on including navigation as a tab, but decided it's probably best not to add it right now. Then it also gets confusing as to what qualifies as navigation and what doesn't. We don't want to make too many assumptions on how things should be named and grouped. If someone wants to make a group for "Lightbox" styles, they can.

I'm still not entirely happy with the "Styles" name for the group and have been trying to come up with a decent name to separate the presentational container styles from general content styles like lists, margins, paddings and other things that might be inside blocks, like more links, forms and buttons, etc. I really would like to separate these, but my mind is blank. This is the best I could come up with (adding general back, but not as the default, and calling "Styles" box style):

/**
 * Implements hook_skinr_group_info().
 */
function skinr_default_skinr_group_info() {
  $group['box'] = array(
    'title' => t('Box style'),
    'description' => t('Presentational styles for the container; default group for skins.'),
  );
  $group['general'] = array(
    'title' => t('General'),
    'description' => t('Styles for content such as lists, buttons, margins, padding, etc.'),
  );
  $group['typography'] = array(
    'title' => t('Typography'),
    'description' => t('Fonts, styles, sizes and other typography related skins.'),
  );
  $groups['layout'] = array(
    'title' => t('Layout'),
    'description' => t('Grid, layout and other structural related skins.'),
  );
  return $groups;
}

I also removed the advanced and information groups for now, because I don't know if they are needed here or not.

ChrisBryant’s picture

Status: Needs review » Reviewed & tested by the community

Looks great to me, thanks Jacine!

jacine’s picture

Status: Reviewed & tested by the community » Fixed
moonray’s picture

Status: Fixed » Needs review
StatusFileSize
new799 bytes

Looks like we missed some typos... $group instead of $groups.
Here's the patch.

jacine’s picture

Status: Needs review » Fixed

Thanks @moonray! Committed: http://drupal.org/cvs?commit=470880

Status: Fixed » Closed (fixed)

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