As per @kslonka's previous patch, this is a smaller patch which removes the unused versions and updates the CDN location

Files: 
CommentFileSizeAuthor
#10 bootstrap-update-cdn-2071683-10.patch6.44 KBheylookalive
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#8 bootstrap-update-cdn-2071683-8.patch6.47 KBheylookalive
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#5 bootstrap-update-cdn-2071683-5.patch6.35 KBheylookalive
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#3 bootstrap-3-update-cdn-2071683-3.patch2.09 KBheylookalive
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
bootstrap-7.x-3.x-cdn.patch1.73 KBtr33m4n
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

Comments

Status:Active» Needs work

Shouldn't you change the 'cdn_bootstrap_version' setting in the .info file to 3.0.0?

+++ b/includes/theme.inc
@@ -278,7 +278,7 @@ function bootstrap_css_alter(&$css) {
   if (theme_get_setting('cdn_bootstrap')){
...
+    $cdn = '//netdna.bootstrapcdn.com/bootstrap/'. theme_get_setting('cdn_bootstrap_version')  .'/css/bootstrap.min.css';
@@ -343,7 +343,7 @@ function bootstrap_js_alter(&$js) {
   if (theme_get_setting('cdn_bootstrap')) {
...
+    $cdn = '//netdna.bootstrapcdn.com/bootstrap/'. theme_get_setting('cdn_bootstrap_version')  .'/js/bootstrap.min.js';
+++ b/theme-settings.php
@@ -46,14 +46,7 @@ function bootstrap_form_system_theme_settings_alter(&$form, $form_state, $form_i
     '#options' => array(
-      '2.3.2' => 'v2.3.2',
-      '2.3.1' => 'v2.3.1',
-      '2.3.0' => 'v2.3.0',
-      '2.2.2' => 'v2.2.2',
-      '2.2.1' => 'v2.2.1',
-      '2.2.0' => 'v2.2.0',
-      '2.1.1' => 'v2.1.1',
-      '2.1.0' => 'v2.1.0',
+      '3.0.0' => 'v3.0.0',
     ),
     '#default_value' => theme_get_setting('cdn_bootstrap_version'),

I would also like to take this opportunity to change and remove unnecessary variable names too. In reality it should be just a simple:

bootstrap_cdn

This variable would be "enabled" if that variable has a version in it and "disabled" if the variable is empty.

We should also use the #empty_option property in the select field.

StatusFileSize
new2.09 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

Updated patch changing the setting in the .info.

I'd say for the sake of one extra variable for the CDN it's not worth removing the checkbox considering the clarity it brings, but if there's a strong feeling about it can roll a patch removing it.

@heylookalive, yes. I would like to turn this into a select drop down with "- None -" at the top of the selection list. There really is no point in having two variables to set (ie: one to "enable" it and one to select the version). There should just be one variable that has the version or is empty (thus disabling the CDN). I really appreciate you stepping up with this btw :) Once these changes have been made, I'll commit.

StatusFileSize
new6.35 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

No worries :)

Here we go, one var, have reworked that form slightly and gone for a more explicit empty option label, reworked a few bits of text and done a few coding standards bits.

Status:Needs work» Needs review

Status:Needs review» Needs work
  1. +++ b/theme-settings.php
    @@ -9,54 +9,31 @@ function bootstrap_form_system_theme_settings_alter(&$form, $form_state, $form_i
    -    '#type'          => 'fieldset',
    -    '#title'         => t('Theme development settings'),
    +    '#type' => 'fieldset',
    +    '#title' => t('Theme development settings'),
    ...
    -    '#type'          => 'fieldset',
    -    '#title'         => t('Theme cdn settings'),
    +    '#type' => 'fieldset',
    +    '#title' => t('Theme CDN settings'),
    +    '#description' => t('Use CDN (Content Distribution Network) to host the bootstrap files, Bootstrap Theme will not use the local CSS files anymore and instead the visitor will download them from <a href="!bootstrapcdn_url">bootstrapcdn.com</a>.', array('!bootstrapcdn_url' => url('http://bootstrapcdn.com'))) . '<div class="alert alert-error">' . t('WARNING: this technique will give you a performance boost but will also make you dependant on a third party who has no obligations towards you concerning uptime and service quality.') . '</div>',

    Now that I think about this and see the code, we might as well just group all these settings into something like a "Bootstrap settings" fieldset with the key value of $form['bootstrap'].

    It's really kind of bad UX to duplicate both a "Theme CDN settings" title and then below show it again for the actual select field. We could move the #description down to the select field.

  2. +++ b/theme-settings.php
    @@ -9,54 +9,31 @@ function bootstrap_form_system_theme_settings_alter(&$form, $form_state, $form_i
    +    '#title' => t('Version of Bootstrap to load from CDN'),

    I would just label this as "BootstrapCDN version"

  3. +++ b/theme-settings.php
    @@ -9,54 +9,31 @@ function bootstrap_form_system_theme_settings_alter(&$form, $form_state, $form_i
    +      '3.0.0' => 'v3.0.0',

    Let's drop the "v" prefix and just do:

    drupal_map_assoc(array(
      '3.0.0',
    )),
  4. +++ b/theme-settings.php
    @@ -9,54 +9,31 @@ function bootstrap_form_system_theme_settings_alter(&$form, $form_state, $form_i
    +    '#empty_option' => t('Do not load from CDN'),

    I'd really rather this just be "Disabled"

Status:Needs work» Needs review
StatusFileSize
new6.47 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

Have made the above changes, I think the form item label for the CDN dropdown has become less explicit. It's clearly explained in the readme and field description but people tend to not look at those things...

Status:Needs review» Needs work
  1. +++ b/bootstrap_subtheme/bootstrap_subtheme.info.starterkit
    @@ -38,7 +38,7 @@ stylesheets[all][] = css/style.css
    -; settings[cdn_bootstrap] = 0
    +; settings[bootstrap_cdn] = ¶

    Is there a reason for this change? Also, there is a space at EOL.

  2. +++ b/theme-settings.php
    @@ -8,55 +8,27 @@ function bootstrap_form_system_theme_settings_alter(&$form, $form_state, $form_i
    +    '#title' => t('Theme CDN settings'),

    Let's just title this "BootstrapCDN". The description explains the title.

  3. +++ b/theme-settings.php
    @@ -8,55 +8,27 @@ function bootstrap_form_system_theme_settings_alter(&$form, $form_state, $form_i
    +    '#description' => t('Use CDN (Content Distribution Network) to host the bootstrap files, Bootstrap Theme will not use the local CSS files anymore and instead the visitor will download them from <a href="!bootstrapcdn_url">bootstrapcdn.com</a>.', array('!bootstrapcdn_url' => url('http://bootstrapcdn.com'))) . '<div class="alert alert-error">' . t('WARNING: this technique will give you a performance boost but will also make you dependant on a third party who has no obligations towards you concerning uptime and service quality.') . '</div>',

    How about we change this to "Use the BootstrapCDN (content distribution network) to serve the Bootstrap framework files. Enabling this will prevent this theme from trying to load local files."

    edit: keep the warning after too though.

Status:Needs work» Needs review
StatusFileSize
new6.44 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

The above changes made, I'm happy with this.

On point one that's a change for the switch in variable names, and was meant to be an empty value which I've replaced with '';

Status:Needs review» Fixed

@heylookalive thanks!

Committed: 16dd99f

Yesss! :)

Status:Fixed» Closed (fixed)

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

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