Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markhalliwell’s picture

Status: Active » Needs work
  1. +++ b/includes/form.inc
    @@ -214,6 +214,7 @@ function bootstrap_pre_render_element($element, &$form_state) {
    +    'checkboxes',
    

    Already committed.

  2. +++ b/template.php
    @@ -91,6 +91,7 @@ function bootstrap_breadcrumb($variables) {
    +    $breadcrumb[] = array('data' => drupal_get_title(), 'class' => array('active'));
    

    I'm not entirely sure we want to do this here. There are modules that can modify the breadcrumbs (including adding the active (current page) at the end. Maybe it should be done in preprocess first?

valkum’s picture

Did a bit of work. Looked ad Zen theme and added some settings.
* Show Breadcrumbs or not or only on admin section
* Show Home Link or not
* Add content title

markhalliwell’s picture

  1. +++ b/template.php
    @@ -83,6 +83,32 @@ function bootstrap_theme(&$existing, $type, $theme, $path) {
    + * Alter breadcrumbs before processing according to theme settings. ¶
    

    Whitespace at EOL.

  2. +++ b/theme-settings.php
    @@ -31,4 +31,30 @@ function bootstrap_form_system_theme_settings_alter(&$form, $form_state, $form_i
    +  $form['breadcrumb'] = array(
    +    '#type' => 'fieldset',
    +    '#title' => t('Breadcrumb settings'),
    +  );
    +  $form['breadcrumb']['bootstrap_breadcrumb'] = array(
    +    '#type' => 'select',
    +    '#title' => t('Display breadcrumb'),
    +    '#default_value' => theme_get_setting('bootstrap_breadcrumb'),
    +    '#options' => array(
    +      'yes' => t('Yes'),
    +      'admin' => t('Only in admin section'),
    +      'no' => t('No'),
    +    ),
    +  );
    +  $form['breadcrumb']['breadcrumb_options']['bootstrap_breadcrumb_home'] = array(
    +    '#type' => 'checkbox',
    +    '#title' => t('Show home page link in breadcrumb'),
    +    '#default_value' => theme_get_setting('bootstrap_breadcrumb_home'),
    +  );
    +  $form['breadcrumb']['breadcrumb_options']['bootstrap_breadcrumb_title'] = array(
    +    '#type' => 'checkbox',
    +    '#title' => t('Append the content title to the end of the breadcrumb'),
    +    '#default_value' => theme_get_setting('bootstrap_breadcrumb_title'),
    +    '#description' => t('Useful when the breadcrumb is not placed just before the title.'),
    +  );
    

    I'd rather keep all "bootstrap" settings in one fieldset personally. I don't like having things split out when they're so few settings.

valkum’s picture

* removed whitespace
* moved settings into 'bootstrap' fieldset

markhalliwell’s picture

  1. +++ b/template.php
    @@ -90,8 +114,12 @@ function bootstrap_theme(&$existing, $type, $theme, $path) {
    +  if ($show_breadcrumb == 'yes' || $show_breadcrumb == 'admin' && arg(0) == 'admin') {
    

    Add an extra set of parenthesis around the second condition:
    if ($show_breadcrumb == 'yes' || ($show_breadcrumb == 'admin' && arg(0) == 'admin')) {

  2. +++ b/theme-settings.php
    @@ -31,4 +31,26 @@ function bootstrap_form_system_theme_settings_alter(&$form, $form_state, $form_i
    +  $form['bootstrap']['bootstrap_breadcrumb'] = array(
    +    '#type' => 'select',
    +    '#title' => t('Display breadcrumb'),
    +    '#default_value' => theme_get_setting('bootstrap_breadcrumb'),
    +    '#options' => array(
    +      'yes' => t('Yes'),
    +      'admin' => t('Only in admin section'),
    +      'no' => t('No'),
    +    ),
    +  );
    +  $form['bootstrap']['breadcrumb_options']['bootstrap_breadcrumb_home'] = array(
    +    '#type' => 'checkbox',
    +    '#title' => t('Show home page link in breadcrumb'),
    +    '#default_value' => theme_get_setting('bootstrap_breadcrumb_home'),
    +  );
    +  $form['bootstrap']['breadcrumb_options']['bootstrap_breadcrumb_title'] = array(
    +    '#type' => 'checkbox',
    +    '#title' => t('Append the content title to the end of the breadcrumb'),
    +    '#default_value' => theme_get_setting('bootstrap_breadcrumb_title'),
    

    Anyway to get #states implemented here that way the checkboxes are hidden if "No" is selected? Also the extra ['breadcrumb_options'] key is no longer needed.

  3. We want to add the default values of these new theme settings to the .info file.
valkum’s picture

Assigned: Unassigned » valkum
valkum’s picture

Status: Needs work » Needs review
FileSize
3.78 KB
markhalliwell’s picture

  1. +++ b/bootstrap.info
    @@ -13,6 +13,9 @@ regions[footer]         = 'Footer'
    +settings[bootstrap_breadcrumb] = yes
    
    +++ b/theme-settings.php
    @@ -31,4 +31,36 @@ function bootstrap_form_system_theme_settings_alter(&$form, $form_state, $form_i
    +      'yes' => t('Yes'),
    +      'admin' => t('Only in admin section'),
    +      'no' => t('No'),
    ...
    +        ':input[name="bootstrap_breadcrumb"]' => array('value' => 'no'),
    ...
    +        ':input[name="bootstrap_breadcrumb"]' => array('value' => 'no'),
    

    Didn't notice this till just now (sorry). Let's keep the keys integers (easier to manage):

    0 => t('No'),
    1 => t('Yes'),
    2 => t('Admin areas only'),
    
  2. +++ b/template.php
    @@ -83,14 +83,40 @@ function bootstrap_theme(&$existing, $type, $theme, $path) {
    +debug($breadcrumb);
    ...
    +debug($breadcrumb);
    

    Forgot to take out debug.

  3. +++ b/template.php
    @@ -83,14 +83,40 @@ function bootstrap_theme(&$existing, $type, $theme, $path) {
    +  $show_breadcrumb = theme_get_setting('bootstrap_breadcrumb');
    +  if (($show_breadcrumb == 'yes' || ($show_breadcrumb == 'admin' && arg(0) == 'admin')) && !empty($breadcrumb)) {
    

    Minor, but has more semantic impact if we just rename $show_breadcrumb to $bootstrap_breadcrumb.

  4. +++ b/theme-settings.php
    @@ -31,4 +31,36 @@ function bootstrap_form_system_theme_settings_alter(&$form, $form_state, $form_i
    +  $form['bootstrap']['bootstrap_breadcrumb_home'] = array(
    +    '#type' => 'checkbox',
    +    '#title' => t('Show home page link in breadcrumb'),
    

    I would put in the following #description here:

    "If your site has a module dedicated to handling breadcrumbs already, ensure this setting is enabled."

  5. +++ b/theme-settings.php
    @@ -31,4 +31,36 @@ function bootstrap_form_system_theme_settings_alter(&$form, $form_state, $form_i
    +  $form['bootstrap']['bootstrap_breadcrumb_title'] = array(
    +    '#type' => 'checkbox',
    +    '#title' => t('Append the content title to the end of the breadcrumb'),
    

    I would put in the following #description here:

    "If your site has a module dedicated to handling breadcrumbs already, ensure this setting is disabled."

markhalliwell’s picture

Status: Needs review » Needs work
markhalliwell’s picture

FYI, just committed b97dbd2 to 7.x-3.x, which is just some coding standards and verbiage cleanup on theme-settings.php. You'll need to re-roll this patch.

valkum’s picture

Status: Needs work » Needs review
FileSize
3.5 KB

here it is

markhalliwell’s picture

Status: Needs review » Needs work

Awesome! Thanks @valkum! Just one tiny nitpick:

+++ b/theme-settings.php
@@ -50,4 +50,36 @@ function bootstrap_form_system_theme_settings_alter(&$form, $form_state, $form_i
+    '#description' => t('If your site has a module dedicated to handling breadcrumbs already, ensure this setting is enabled.'),
...
+    '#description' => t('If your site has a module dedicated to handling breadcrumbs already, ensure this setting is <b>disabled</b>.'),

I wasn't actually intending for this to be bold, but in retrospect I actually like it. It should be <strong> however. Also go ahead and wrap the "enabled" one above too.

valkum’s picture

valkum’s picture

Status: Needs work » Needs review
markhalliwell’s picture

Status: Needs review » Needs work

Ok, I just did a manual review now. Two minor things:

  1. +++ b/template.php
    @@ -95,7 +119,9 @@ function bootstrap_theme(&$existing, $type, $theme, $path) {
    +  // Determine if we are to display the breadcrumb.
    +  $bootstrap_breadcrumb = theme_get_setting('bootstrap_breadcrumb');
    +  if (($bootstrap_breadcrumb == 1 || ($bootstrap_breadcrumb == 2 && arg(0) == 'admin')) && !empty($breadcrumb)) {
    

    The top level page does not have breadcrumbs. I'm not entirely sure if this is by design or not. If it is, then that's fine. Otherwise this probably needs to be refactored.

  2. +++ b/theme-settings.php
    @@ -50,4 +50,36 @@ function bootstrap_form_system_theme_settings_alter(&$form, $form_state, $form_i
    +        ':input[name="bootstrap_breadcrumb"]' => array('value' => 1),
    ...
    +        ':input[name="bootstrap_breadcrumb"]' => array('value' => 1),
    

    This needs to be 0 now.

valkum’s picture

As the titlefor Frontpage ist empty adding the title through

$item = menu_get_item();
    if (!empty($item['tab_parent'])) {
      // If we are on a non-default tab, use the tab's title.
      $breadcrumb[] = check_plain($item['title']);
    }
    else {
      $breadcrumb[] = drupal_get_title();
    }

results in an array([0] => '');
And this will render to just the background.
If you show the home page link it should be fine.

valkum’s picture

Status: Needs work » Needs review
markhalliwell’s picture

Status: Needs review » Fixed

Thanks @valkum!

Committed 7bc02a7 to 7.x-3.x.
Committed b9e5144 to 7.x-3.x.

markhalliwell’s picture

Version: 7.x-3.x-dev » 7.x-3.0-rc1

Status: Fixed » Closed (fixed)

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