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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

olamaekle’s picture

Status: Active » Needs work

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

markhalliwell’s picture

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

heylookalive’s picture

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.

markhalliwell’s picture

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

heylookalive’s picture

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.

heylookalive’s picture

Status: Needs work » Needs review
markhalliwell’s picture

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"

heylookalive’s picture

Status: Needs work » Needs review
FileSize
6.47 KB

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

markhalliwell’s picture

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.

heylookalive’s picture

Status: Needs work » Needs review
FileSize
6.44 KB

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 '';

markhalliwell’s picture

Status: Needs review » Fixed

@heylookalive thanks!

Committed: 16dd99f

heylookalive’s picture

Yesss! :)

Status: Fixed » Closed (fixed)

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

markhalliwell’s picture

Version: 7.x-3.x-dev » 7.x-3.0-beta1
nithinkolekar’s picture

Issue summary: View changes
FileSize
71.37 KB

@markcarver
If CDN setting is set to "-none-" and bootswatch css,js files are added locally to subtheme , should it be mandatory to have core bootstrap css file too?

Bootswatch css file ~145kb which is same as core css file and I thought it has all the code as core but with style modification. Is that correct?

main issue: with the above configuration admin menu is overlapped by nav bar. If cdn value is set to 3.3.5 or any other value everything works fine.(~300kb in total for two files).

cdn-none-issue

nithinkolekar’s picture

update on #15
/sites/all/themes/bootstrap/css/3.3.5/overrides.min.css?p07ytg is not loading when CDN set to none and I tested it by copying css code and applied in firefox style editor then nav bar shifts down properly.

markhalliwell’s picture

files are added locally to subtheme , should it be mandatory to have core bootstrap css file too?

No. The compiled overrides CSS only shows up if you use the CDN (because we can't control what the CDN serves).

The intended purpose of doing any "local" files is to utilize (and change) the source files (LESS or SASS) to create a custom theme. The LESS and SASS starterkits come with the overrides source files that you can customize and compile into your sub-theme.

If you're using a "local" compiled copy of Bootstrap/Bootswatch, then you'd be better off just using the CDN in the first place. If you cannot, for whatever reason, then you will need to copy the appropriate compiled overrides CSS from the base theme into your sub-theme and add it to the *.libraries.yml file.