The type select list is totally broken. Some observations:

  1. The regions for every theme, whether disabled or not are showing. We should only have enabled themes here, unless there is a good reason. I don't know that there is one, because even sub themes do not inherit regions automatically.
  2. The regions are all thrown in the list in no logical order, and with no respect for differences between themes. For example, if theme (a) has regions[sidebar_first] = Sidebar first and theme (b) has regions[sidebar_first] = First sidebar, this list will show the one that loads last.
  3. I don't think we are accounting for themes that use core default regions in this list, e.g. Garland, Stark.

I think the solution here is to create a select with an optgroup:

<select id="edit-rule-type" name="rule_type" class="form-select required">
  <option value="page">Page</option>
  <optgroup label="Bartik regions">
    <option value="region__header">Header</option>
    <option value="region__help">Help</option>
    <option value="region__highlighted">Highlighted</option>
    <option value="region__content">Content</option>
    <option value="region__sidebar_first">Sidebar first</option>
    <option value="region__sidebar_second">Sidebar second</option>
    <option value="region__triptych_first">Triptych first</option>
    <option value="region__triptych_middle">Triptych middle</option>
    <option value="region__triptych_last">Triptych last</option>
    <option value="region__footer_firstcolumn">Footer first column</option>
    <option value="region__footer_secondcolumn">Footer second column</option>
    <option value="region__footer_thirdcolumn">Footer third column</option>
    <option value="region__footer_fourthcolumn">Footer fourth column</option>
  </optgroup>
  <optgroup label="Seven regions">
    <option value="region__content">Content</option>
    <option value="region__help">Help</option>
  </optgroup>
  <optgroup label="Stark regions">
    <option value="region__content">Content</option>
    <option value="region__footer">Content</option>
    <option value="region__header">Header</option>
    <option value="region__help">Help</option>
    <option value="region__highlighted">Highlighted</option>
    <option value="region__sidebar_first">Sidebar first</option>
    <option value="region__sidebar_second">Sidebar second</option>
  </optgroup>
  <optgroup label="Garland regions">
    <option value="region__content">Content</option>
    <option value="region__footer">Content</option>
    <option value="region__header">Header</option>
    <option value="region__help">Help</option>
    <option value="region__highlighted">Highlighted</option>
    <option value="region__sidebar_first">Sidebar first</option>
    <option value="region__sidebar_second">Sidebar second</option>
  </optgroup>
</select>

Basically, a list of regions that are not hidden grouped by themes, and ordered alphabetically.

CommentFileSizeAuthor
#17 942950-17.patch3.05 KBjacine
#15 Regions-before.png24.61 KBvrajak@gmail.com
#15 Regions-Patched.png95.29 KBvrajak@gmail.com
#11 skinr_942950_11.patch2.83 KBmoonray
#7 942950-7.patch1.87 KBjacine
#7 skinr-rule-create.png34.62 KBjacine
#6 rule-creation-select.jpg91.36 KBnomonstersinme
#5 skinr_942950.patch1.55 KBmoonray
#2 skrin_ui.rules_.patch863 bytesmoonray
rules-7.x.png153.14 KBjacine

Comments

jacine’s picture

jacine
she/her
Location New York
commented
Assigned: Unassigned » moonray

@moonray said he was working on this, on Friday, so assigning it to him in the meantime so no one else works on it.

moonray’s picture

moonray commented
Status: Active » Needs review
StatusFileSize
new skrin_ui.rules_.patch863 bytes

Here's that patch.

jacine’s picture

jacine
she/her
Location New York
commented
Assigned: moonray » jacine
Status: Needs review » Needs work

I'm still getting disabled themes in this list and I don't see the point of showing their regions here. To modify any theme settings the theme needs to be enabled, so we should follow the same logic. I'd like to remove the "Region: " and keep it in the option group label, because it's too redundant.

I'm going to give it a shot.

jacine’s picture

jacine
she/her
Location New York
commented

There's no need for a fieldset here, so I'm removing that. Also noting that there is an issue with the hook_menu() types. I am not getting proper titles where I should, so trying to figure out why.

moonray’s picture

moonray commented
Status: Needs work » Needs review
StatusFileSize
new skinr_942950.patch1.55 KB

New patch.
Fieldset removed, and disabled themes no longer show in the list.

nomonstersinme’s picture

nomonstersinme commented
StatusFileSize
new rule-creation-select.jpg91.36 KB

This is looking good to me!

Edit: re-tested with drupal head and it looks the same as the photo

jacine’s picture

jacine
she/her
Location New York
commented
StatusFileSize
new skinr-rule-create.png34.62 KB
new 942950-7.patch1.87 KB

As I mentioned in the original issue, I'd like to remove the redundant "Regions:" text from the label:

  <optgroup label="Bartik regions">
    <option value="region__header">Header</option>

Here's an updated patch (with comments), and a screenshot of what it looks like.

nomonstersinme’s picture

nomonstersinme commented
Status: Needs review » Reviewed & tested by the community

This looks just like your png on my set up :)

moonray’s picture

moonray commented

Looks good to me. RTBC+

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Needs work 
+++ skinr_ui.rules.inc	23 Oct 2010 17:09:44 -0000
@@ -57,13 +57,7 @@ function skinr_rules() {
-  $form['rule']['title'] = array(
+  $form['title'] = array(

It would be a good idea to keep the additional top-level 'rule' parent key, even though the fieldset is removed. That potentially allows other modules to extend the form.

Speaking of, it would be a good idea to set

$form['#tree'] = TRUE;

and adjust the form's validation and submit handlers to act on $form_state['values']['rule'] instead.

+++ skinr_ui.rules.inc	23 Oct 2010 17:09:44 -0000
@@ -71,15 +65,20 @@ function skinr_rule_add($form, &$form_st
+    $options_region = array();
+    // Create a list options containing visible regions of enabled themes.

1) $regions would be a better variable name.

2) Ideally put the comment above the initialization of $regions = array().

3) That comment reads a bit odd. Also, it talks about multiple themes, but the code only processes regions of one theme.

+++ skinr_ui.rules.inc	23 Oct 2010 17:09:44 -0000
@@ -71,15 +65,20 @@ function skinr_rule_add($form, &$form_st
+    foreach (system_region_list($key, REGIONS_VISIBLE) as $rkey => $region) {

Regions are actually keyed by $name and their value is a $title.

+++ skinr_ui.rules.inc	23 Oct 2010 17:09:44 -0000
@@ -71,15 +65,20 @@ function skinr_rule_add($form, &$form_st
+    $options[$theme->info['name'] . ' ' . t('Regions')] = $options_region;

Both the old and the new code use t() incorrectly.

t() should contain the entire string, using placeholders for replacement variables, so as to allow translators to understand that "Regions" is actually part of a larger string (which potentially can appear in front of the theme name in some languages).

Thus:

$key = t('@name regions', array('@name' => $theme->info['name']));

Note: I didn't look up whether @name is the correct placeholder token type. It's possible that we need to use !name, if http://api.drupal.org/api/function/form_select_options/7 already calls check_plain() for the optgroup.

However, looking at it as I'm typing, it seems like it's taking over the key as-is, so @name should be correct.

Powered by Dreditor.

moonray’s picture

moonray commented
Status: Needs work » Needs review
StatusFileSize
new skinr_942950_11.patch2.83 KB

Here's a patch that addresses all those issues.

Status: Needs review » Needs work

The last submitted patch, skinr_942950_11.patch, failed testing.

nomonstersinme’s picture

nomonstersinme commented
Status: Needs work » Needs review

This looks good to me visually.. code wise i dunno ;)

jacine’s picture

jacine
she/her
Location New York
commented

#11: skinr_942950_11.patch queued for re-testing.

vrajak@gmail.com’s picture

vrajak@gmail.com commented
StatusFileSize
new Regions-Patched.png95.29 KB
new Regions-before.png24.61 KB

Patch works just fine for me, and certainly cleans up the Rule selection list. I don't know why the patch failed testing, it applied without any errors on my setup. I can't comment on any coding, but as nomonstersinme said it looks good visually.

Attaching "before" and "after" patch photos just for reference.

jacine’s picture

jacine
she/her
Location New York
commented
Status: Needs review » Needs work

Hm, this no longer applies. I'll reroll.

jacine’s picture

jacine
she/her
Location New York
commented
Status: Needs work » Fixed
StatusFileSize
new 942950-17.patch3.05 KB

Committed: http://drupal.org/cvs?commit=458992

Status: Fixed » Closed (fixed)

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