from sun's issue here: #849862: Bartik code problems

+++ themes/bartik/bartik.info 6 Jul 2010 03:48:59 -0000
@@ -0,0 +1,39 @@
+regions[highlight] = Highlighted
...
+regions[featured] = Featured

One of these region names should be changed to match the tense of the other -- i.e., either highlight and feature, or highlighted and featured. I'd prefer the latter.

CommentFileSizeAuthor
#5 drupal.highlighted.5.patch6.7 KBsun
#3 highlighted.patch6.17 KBjacine

Comments

jensimmons’s picture

jensimmons commented
Component: markup » Bartik theme
Category: task » bug
Priority: Normal » Minor

As asimmonds pointed out here: http://drupal.org/node/849862#comment-3185158 I followed what is in core, using 'highlight' for the var. 

  // Set defaults for theme info.
  $defaults = array(
    'regions' => array(
      'sidebar_first' => 'Left sidebar',
      'sidebar_second' => 'Right sidebar',
      'content' => 'Content',
      'header' => 'Header',
      'footer' => 'Footer',
      'highlight' => 'Highlighted content',
      'help' => 'Help',
      'page_top' => 'Page top',
      'page_bottom' => 'Page bottom',
    ),

I agree with sun, I prefer 'featured' to 'feature' — really, they mean very different things. A piece of content is featured; something the tool does is a feature.

I didn't want to change 'highlight' in case something in core or in contrib was looking for that preexisting region for placing something automatically. While consistency in naming variables is preferred, having consistency between core and the Bartik theme is more important, imo.

Remember, the variable names are not exposed to the GUI interface user. They'll just see the labels.

Should we change "highlight" to "highlighted" in core itself? And change it in Bartik too?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Bartik: rename 'highlight' region 'highlighted' » Consider renaming 'highlight' region to 'highlighted'
Component: Bartik theme » markup
Category: bug » task
Priority: Minor » Normal

Thanks for your thorough analysis! Since "highlight" was introduced for D7 (I think), we should ideally fix it to "highlighted" throughout core. It would be wrong to introduce this wrong name, resp. mismatch.

jacine’s picture

jacine
she/her
Location New York
commented
Component: Bartik theme » markup
Category: bug » task
Priority: Minor » Normal
Status: Active » Needs review
StatusFileSize
new highlighted.patch6.17 KB

Yes, agreed. Here's a patch.

Status: Needs review » Needs work

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

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal.highlighted.5.patch6.7 KB

.info file?

jacine’s picture

jacine
she/her
Location New York
commented

Yes, .info file would help. Guess my edit didn't stick :P Sorry about that.

Looks good now.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Aight.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

pwolanin’s picture

pwolanin commented

Should be rolled back - impacts any contrib themes already ported to D7. This is way past API freeze and causes pain for no technical reason.

Also - longer class names == bandwidth suck.

David_Rothstein’s picture

David_Rothstein commented
Status: Fixed » Needs work
Issue tags: +Needs documentation

This requires a documentation update at http://drupal.org/node/254940#highlight-region

Given that this is not a required theme region, I don't see it as an actual API change. Maybe a "pseudo" API change in the sense that themes which had previously tried to follow core (either partially or, via not defining their own regions and falling back on the defaults, completely) need to either decide to stop following core's lead or else update their theme. But there's nothing here that prevents a theme sticking with 'highlighted' [edit: 'highlight'] if it wants to.

jacine’s picture

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

I updated the docs here and here.

FTR, I don't think this should be rolled back.

David_Rothstein’s picture

David_Rothstein commented

I reviewed the doc changes - they mostly looked good, although the new instructions seem to assume that your theme already defines its own custom regions (and therefore needs to add this to its info file specifically)...

But if you have a theme which (like Garland) just inherits the core defaults, then I believe you don't have to do anything to your .info file at all to pick up this change; rather, you get it automatically, and only have to do something to your info file if you want to specifically avoid having it, right?

jacine’s picture

jacine
she/her
Location New York
commented

Ok, I tried to clarify. Hopefully it's better :)

David_Rothstein’s picture

David_Rothstein commented
Status: Needs review » Fixed

Sorry for the delayed response :)

The new docs look good. In the interim, someone else had come along after your edits and changed one of the docs back to say 'highlight' (I have no idea why?), so I fixed that again to say 'highlighted'. I think this is well-documented now, and we can close it.

Status: Fixed » Closed (fixed)
Issue tags: -Needs documentation

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

Issue tags: +Needs documentation

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.