Breaking off per #1844448-48: Add sub-theme starter kit. Code needs to be refactored so if a Bootstrap sub-theme has chosen to use the CDN, it will add the appropriate CSS & JS files via the Bootstrap base theme , otherwise it shouldn't add anything.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markhalliwell’s picture

Attaching patch. The CDN is enabled by default (to provide the "out-of-box" experience). I also went ahead and added the latest 2.3.1 version. Essentially, the base theme no longer provides support (other than the CDN) for adding the Bootstrap library CSS and JS files. If a sub-theme chooses to use source files (compiled or otherwise), then it will need to download the Bootstrap library in it's sub-folder, disable the CDN setting in it's .info file and provide it's own CSS and JS declarations.

Also, I was tempted to put this comment in includes/theme.inc line 296 regarding the bootstrap_ui module. I ultimately didn't as there are already issues regarding this, so I'll just reference it here (the README should probably be updated after a solution to these problems exists):

  // @TODO The following code should be re-evaluate per:
  // #1852332: Integrate with the Libraries API module and #1838982: twitter_bootstrap_ui module.
  // Technically speaking "bootstrap_ui" doesn't exist,
  // "twitter_bootstrap_ui" and "bootstrap_api" do though.
  // It should also be noted there is no similar task for CSS and this is being
  // called in an alter hook, which begs the question if this actually would
  // get loaded.
  if (module_exists('bootstrap_ui')) {
    libraries_load('bootstrap', 'minified');
  }
markhalliwell’s picture

Status: Active » Needs review

Forgot to change the status.

Also, should be mentioned that this patch is after changes made in #1846736: Remove jQuery CDN setting in favor of using jQuery Update module.

Status: Needs review » Needs work

The last submitted patch, 1957620-bootstrap-refactor_code_cdn-1.patch, failed testing.

markhalliwell’s picture

Yeah... I thought this might happen. Will need to re-test after #1846736: Remove jQuery CDN setting in favor of using jQuery Update module has been committed to dev.

markhalliwell’s picture

Status: Needs work » Needs review
markhalliwell’s picture

wundo’s picture

markhalliwell’s picture

It passed.

wundo’s picture

Status: Needs review » Fixed

Committed, thanks :)

Status: Fixed » Closed (fixed)

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