While using this theme in conjunction with the Configuration module I came across a bug in the _vertical_tabs.js where the theme function was being placed in the Drupal.theme.prototype namespace instead of the Drupal.theme namespace as per the guidelines here:

https://drupal.org/node/132442#javascript-themeing

As far as I'm aware, themes should always use the Drupal.theme namespace so that they properly override any module theme implementations. This was preventing the Bootstrap theme from adding its mojo into the Configuration settings page and breaking the showing of tabs as Bootstrap requires the pane ID in the anchor tab button.

Will attach a patch.

Comments

ryan.armstrong’s picture

Here's the patch. Thanks for all the great work on this theme btw!

ryan.armstrong’s picture

Status: Active » Needs review

Updating status.

markhalliwell’s picture

Status: Needs review » Closed (won't fix)

Normally, I would agree with you here. But you will notice that we're replacing the entire vertical-tabs.js file (the _ at the beginning denotes this), not just overriding some theming functions. See: bootstrap_js_alter().

If we were to make this change the prototype wouldn't exist and could ultimately cause complications down the road. It's also honestly just simpler to mimic what the module in core is doing so we change as little as possible. This will allow us to detect what in the file needs to be updated if the original file is changed in the future.

markhalliwell’s picture

Whoops, didn't mean to "delete" the patch... that is really... odd.

ryan.armstrong’s picture

Hmm well the issue here then is that this will seemingly conflict with any module that tweaks the vertical tabs, like the Configuration module.

https://drupal.org/node/2129299

They did the reverse, where they used Drupal.theme rather then Drupal.theme.prototype. In order for the vertical tabs to work, both the Configuration module as well as the Bootstrap theme have to use the normal namespaces. Not sure if this would be true for other modules that modify the verticalTab (or other theme functions).

In reference to your comment that this is due to the fact that you are replacing the vertical tabs completely, I would expect this to not be an issue. So perhaps what is happening is that you are replacing the core verticalTab.js, then the configuration module is coming along and modifying Bootstrap. Which then breaks bootstrap (It just places a '#' in the tab button a tag rather then the panel ID which bootstrap needs to show/hide panels).

The issue here is that core vertical tabs doesn't require the panel ID while Bootstrap does, thus any module that overrides core will likely use the core method, and thus break when using Bootstrap.