search

localgovdrupal / localgov_base

The base theme for LocalGov Drupal websites.
8 stars 14 forks source link

core/drupal should be a global dependency #557

Closed ctorgalson closed 3 months ago

ctorgalson commented 3 months ago

I'm working on an LGD implementation that can't use the localgov_base header library, so I excluded it with the following in the sub-theme's info file:

libraries-override:
  localgov_base/header-js: false

Immediately after doing this, all javascript in the sub-theme started to fail with Uncaught ReferenceError: Drupal is not defined in the browser console.

I was able to correct the problem in the sub-theme by adding core/drupal to the sub-theme's global library.

Since all Drupal behaviours (including localgov_base/guides) rely on core/drupal, and since sub-themes may replace some/all libraries, IMO core/drupal should probably become a global library.

ctorgalson commented 3 months ago

Granted it's possible that this would create the opposite issue for subthemes that choose to override both localgov_base/header-js and localgov_base/guides without javascript (i.e. those themes would then have an unwanted library, core/drupal included in the page).

Anyhow, if it's useful, I added #558.

markconroy commented 3 months ago

Thanks for posting this issue Christopher. It's an interesting one, but I'm not fully convinced that the solution is to add drupal/core as a global requirement.

How about we add drupal/core as a requirement to localgov_base/guides library instead. Ideally we just want things that are needed loading on pages and not any extra resources.

ctorgalson commented 3 months ago

but I'm not fully convinced

Fair enough. As I said in the comment, it's not automatically the right solution.

How about we add drupal/core as a requirement to localgov_base/guides library instead

I swear I thought it already was (that's why I linked it). Must have been looking at subsites-menu. Since I made the patch anyhow, I'll update it to do only that.