backdrop-contrib / views_bootstrap

This module provides a set of styles for the Views module. It allows you to create responsive Bootstrap 3.x components, such as grids, carousels, tabs, and tables, all within the configuration settings of the powerful Views module.
https://backdropcms.org/project/views_bootstrap
GNU General Public License v2.0
0 stars 7 forks source link

Load Bootstrap assets only for views_bootstrap plugins #28

Closed Radcliffe closed 5 years ago

Radcliffe commented 5 years ago

In #27 @daggerhart wrote:

If I had to guess, it's the inclusion of these css and js assets that are causing all the admin-theme related problems (#4, #7, #24, more?). Theoretically, this module should only change the HTML output of a single view when that view has a views_bootstrap style plugin selected. It should be up to the theme to include the bootstrap assets.

This makes sense to me. Let's do this.

stpaultim commented 5 years ago

Sorry guys, but I think we have a problem.

This change requires that a user check the box to "Include Bootstrap CSS & JS when used by Views." This will be a problem for anyone currently using the module that updates the module, because they will not have this setting checked and their views will break if they don't have a bootstrap theme or are not currently loading bootstrap.

Unless I'm missing something.

We could write an update hook (I assume, but have not done so myself) that checks to see if the module is already installed and if so, either displays a message/notification of this change OR automatically checks this box for them.

I'm a little nervous about including this change in a minor release, unless we turn it on by default and allow a user to turn it off (again, we could implement a notification that informs them of this new option). It seems likely to me that many (of the 33) users may not have Bootstrap loading from another source.

stpaultim commented 5 years ago

@Radcliffe - Do I understand that your new PR simply turns this setting on by default and you are assuming that it will be turned off in a future update?

Radcliffe commented 5 years ago

If the setting does not exist then it is created and set to TRUE. If the setting does exist (whether TRUE or FALSE) then there is no change.

daggerhart commented 5 years ago

Just throwing this out there.

There was previously a setting named "views_bootstrap_visibility" that i removed in my PR. I'm not sure what happens to settings in backdrop when they are removed from a module's settings.json file, but theoretically you could check its value to know whether or not to enable the new setting.

Roughly something like:

/**
 * Add config option to include Bootstrap assets.
 */
function views_bootstrap_update_7100() {
  $should_include_assets = config_get('views_bootstrap.settings', 'views_bootstrap_visibility');
  if ($should_include_assets > 0) {
     config_set('views_bootstrap.settings', 'include_bootstrap_assets', TRUE)
  }
}
daggerhart commented 5 years ago

I think this is good to go. @Radcliffe's approach of checking for a strict NULL value makes perfect sense.