Bartik code problems

Straight copy from #683026-383: Add a new theme to D7 core: Bartik:

Time for a code review:

+++ themes/bartik/bartik.info	6 Jul 2010 03:48:59 -0000
@@ -0,0 +1,39 @@
+package = Core

Do themes have a package?

+++ themes/bartik/bartik.info	6 Jul 2010 03:48:59 -0000
@@ -0,0 +1,39 @@
+stylesheets[all][] = css/maintenance-page.css

Why is the maintenance page stylesheet loaded on all regular pages?

+++ 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.

+++ themes/bartik/template.php	6 Jul 2010 03:48:59 -0000
@@ -0,0 +1,146 @@
+function bartik_process_page(&$variables) {
...
+function bartik_process_maintenance_page(&$variables) {

Both process functions partially contain identical lines, i.e., duplicate code. Those lines should be moved into a dedicated private helper function, e.g., _bartik_process_page(&$variables).

+++ themes/bartik/template.php	6 Jul 2010 03:48:59 -0000
@@ -0,0 +1,146 @@
+  // Always print the site name and slogan, but if they are toggled off, we'll
+  // just hide them visually.
+  $variables['hide_site_name']   = theme_get_setting('toggle_name') ? FALSE : TRUE;
+  $variables['hide_site_slogan'] = theme_get_setting('toggle_slogan') ? FALSE : TRUE;

These variables (and others) should be negated, i.e., into "show_xyz" or "toggle_xyz".

I've personally implemented and used similar "hide_xyz" template variables in custom themes some time ago, only to find out that a constant double negation of "hide foo" => FALSE ===> yes, TRUE is a super major source of confusion.

If you look closely at these lines, then you hopefully realize this problem already. Removing the double negation would lead to simplicity in

$variables['toggle_name'] = (bool) theme_get_setting('toggle_name');

Additionally, it slightly hurts my brain to see new flag names being introduced here, which require everyone to map "toggle_name" to "hide_site_name" and vice-versa. Effectively, not only a different name, but also negated meaning.

Final note for D8: We should auto-provision theme settings into a dedicated $variables['settings'] key for all templates by default, so as to simplify and remove all of this code from Bartik.

+++ themes/bartik/template.php	6 Jul 2010 03:48:59 -0000
@@ -0,0 +1,146 @@
+  // In the header region, visually hide the title of any menu block or of the
+  // user login block, but leave it accessible.
+  if ($variables['block']->region == 'header' && ($variables['block']->module == 'menu' || $variables['block']->module == 'user' && $variables['block']->delta == 'login')) {
+    $variables['title_attributes_array']['class'][] = 'element-invisible';
+  }

A problem of this approach is that sighted users will not see the actual heading that is output on the page, and therefore, they will not even think of the possibility that there might be an invisible heading that may be read by someone, which might use a better title than "[paste some Drupalism here]".

Also, this is not what the .element-invisible class has been designed for. I'd recommend to just forcefully remove the block title instead, so as to not output it at all.

+++ themes/bartik/template.php	6 Jul 2010 03:48:59 -0000
@@ -0,0 +1,146 @@
+  // System menu blocks should get the same class as menu module blocks.
+  if (in_array($variables['block']->delta, array_keys(menu_list_system_menus()))) {

Faster:

if (($menus = menu_list_system_menus()) && isset($menus[$block->delta])) {

Also, same .element-invisible issue applies here.

+++ themes/bartik/template.php	6 Jul 2010 03:48:59 -0000
@@ -0,0 +1,146 @@
+function bartik_page_alter(&$page) {
+  // Determine the position and count of blocks within regions.

I'm not sure why this code is needed. I'm pretty sure that this information already exists in $variables, and always existed since at least Drupal 5.

If any information is indeed missing or no longer the case, then we have to adjust Block module's block rendering code instead. In a separate issue, with high/major priority.

+++ themes/bartik/color/preview.css	6 Jul 2010 03:48:59 -0000
@@ -0,0 +1,62 @@
+/* Bring in the rest of the theme's CSS styles. */
...
+/* ---------- Basic Preview Styles ----------- */

Please consistently use the first style for non-block CSS comments.

To form a CSS "chapter" or section, use a full block style comment, i.e.:

/**
 * Basic preview styles.
 */

Also note regular sentence capitalization and punctuation.

+++ themes/bartik/color/preview.css	6 Jul 2010 03:48:59 -0000
@@ -0,0 +1,62 @@
+  width: 960px;
+  margin-left: auto;
+  margin-right: auto;
+  padding: 0 20px;

Unfortunately, almost everywhere:

According to most current Drupal CSS coding standards, styles need to be ordered alphabetically. See http://drupal.org/node/302199

+++ themes/bartik/css/maintenance-page.css	6 Jul 2010 03:49:00 -0000
@@ -0,0 +1,62 @@
+.maintenance-page #header div.section, ¶
+.maintenance-page #navigation div.section, ¶
+.maintenance-page #messages, ¶

Trailing white-space here.

+++ themes/bartik/css/style-rtl.css	6 Jul 2010 03:49:00 -0000
@@ -0,0 +1,254 @@
+  border-left: 1px solid #555;
+
+  border-right: none;

(and possibly elsewhere) Stray blank line here.

+++ themes/bartik/css/style.css	6 Jul 2010 03:49:00 -0000
@@ -0,0 +1,1326 @@
+.ui-widget {
+  font-family: Georgia, "Times New Roman", Times, serif;

Not sure whether jQuery UI styles belong into style.css. No change request or recommendation at this point; we'll have some major headaches with jQuery UI during D7 anyway ;)

+++ themes/bartik/css/style.css	6 Jul 2010 03:49:00 -0000
@@ -0,0 +1,1326 @@
+/* ----------------- Comments ----------------- */
...
+/* ------------------ Sidebar ----------------- */

Same as above - all of these are clearly style sections and therefore need to use the documentation block syntax. An arbitrary amount of hyphens before and after a randomly centered heading is unmaintainable.

+++ themes/bartik/css/style.css	6 Jul 2010 03:49:00 -0000
@@ -0,0 +1,1326 @@
+fieldset {
...
+.fieldset-wrapper {
...
+.filter-wrapper {
...
+fieldset.collapsed {
...
+fieldset legend {
...
+fieldset.collapsed legend {
...
+fieldset legend a {
...
+fieldset .fieldset-wrapper {

The definition order of some styles looks a bit random to me. Perhaps we can shuffle these a bit around, so as to hierarchically group styles that belong together. That would vastly help in long-term maintenance.

+++ themes/bartik/scripts/search.js	6 Jul 2010 03:49:00 -0000
@@ -0,0 +1,26 @@
+  Drupal.behaviors.bartik = {
+    attach: function(context) {

This JS behavior effectively is http://drupal.org/project/compact_forms in a core theme. You will quickly find out that the code used here does not work in all browsers/platforms/versions. The issue queue of aforementioned project holds more detailed debugging information.

+++ themes/bartik/templates/comment-wrapper.tpl.php	6 Jul 2010 03:49:00 -0000
@@ -0,0 +1,54 @@
+/**
+ * @file
+ * Bartik's theme implementation to provide an HTML container for comments.
+ *
+ * Available variables:

+++ themes/bartik/templates/comment.tpl.php	6 Jul 2010 03:49:00 -0000
@@ -0,0 +1,102 @@
+/**
+ * @file
+ * Bartik's theme implementation for comments.
+ *
+ * Available variables:

If I'm not mistaken, then we are not duplicating the entire template documentation in themes. Just the @file summary, and only optionally, any additional important information about the theme template override -- for example, like here: a short/minimal summary of the differences to the original template, if possible. If not possible, be it, just @file.

+++ themes/bartik/templates/maintenance-page.tpl.php	6 Jul 2010 03:49:00 -0000
@@ -0,0 +1,93 @@
+    </div></div> <!-- /.section, /#content -->
...
+  </div></div> <!-- /#main, /#main-wrapper -->
...
+</div></div> <!-- /#page, /#page-wrapper -->

I actually hoped that a modern 2010 style theme wouldn't contain such markers. Any special reason to include them? Normally, the indentation within a template should sufficiently describe opening and closing sections.

+++ themes/bartik/templates/node.tpl.php	6 Jul 2010 03:49:00 -0000
@@ -0,0 +1,126 @@
+    // Remove the "Add new comment" link on the teaser page or if the comment
+    // form is being displayed on the same page.
+    if ($teaser || !empty($content['comments']['comment_form'])) {
+      unset($content['links']['comment']['#links']['comment-add']);
+    }

Such adjustments should really be done in a preprocess function, not arbitrarily stuffed within a template. All core theme templates should be lean and mean.

+++ themes/bartik/templates/page.tpl.php	6 Jul 2010 03:49:00 -0000
@@ -0,0 +1,280 @@
+        <?php if ($site_slogan): ?>
+          <div id="site-slogan"<?php if ($hide_site_slogan) { print ' class="element-invisible"'; } ?>>
+            <?php print $site_slogan; ?>

As mentioned at the beginning: My brain hurts reading these lines.

Powered by Dreditor.