Give the custom css file a 'name' to make it easier to manipulate in hook_civicrm_alterBundle() #20278
Conversation
|
(Standard links)
|
| $this->assertEquals('civicrm:css/custom.css', $bundle->get('civicrm:css/custom.css')['name']); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
The styleChecker wants you to take out this extra line.
hook_civicrm_alterBundle()
|
A+ for including a test :) |
This means custom CSS also needs to be bumped above 290. Until civicrm/civicrm-core#20278 is merged, there is no easy way to identify which snippet is the custom CSS one so we search for 'custom.css' in the name.
|
This looks like a good idea. Theoretically, sometimes one might be concerned that adding a |
|
ping @haystack @mattwire @jamienovick as you might be interested in this change |
|
@deb1990 any views on this? |
| @@ -97,7 +97,7 @@ public static function createStyleBundle($name) { | |||
| $config = CRM_Core_Config::singleton(); | |||
| if (!empty($config->customCSSURL)) { | |||
| $customCSSURL = Civi::resources()->addCacheCode($config->customCSSURL); | |||
| $bundle->addStyleUrl($customCSSURL, 99); | |||
| $bundle->addStyleUrl($customCSSURL, ['weight' => 99, 'name' => 'civicrm:css/custom.css']); | |||
There was a problem hiding this comment.
Looks like a very good idea. A small suggestion though.
Do we need the :css thingy? How about just making it civicrm:custom.css.
Its not a huge deal, but civicrm:css/custom.css kind of makes it looks like a URL. When civicrm:custom.css looks more of an identifier.
There was a problem hiding this comment.
Ah yes ... what's in a name??
Resources added via addStyleFile() have a default name combining the extension name and the relative path so the other resources added by createStyleBundle() are civicrm:css/civicrm.css and civicrm:css/crm-i.css. So following that pattern of names I went with civicrm:css/custom.css. However,
- the file isn't provided by core so arguably isn't
civicrm- but it is added by core. - the path is not necessarily
css- but is quite likely to be[civicrm.files]/css - the file name is not necessarily
custom.css- but is a common option
Providing the name is unique I don't think it matters much. The purpose is to let people target that custom css file, whatever it's actual filename or path. I did consider civicrm:custom.css but thought the omission of the css/ part might cause more question since the name then does not follow the pattern of the other names in the bundle.
Happy to change the name if there's consensus on a better one.
And while you're all thinking about this, perhaps you could also comment on https://lab.civicrm.org/dev/core/-/issues/2460 which led to this.
There was a problem hiding this comment.
Thanks for the explanation. It makes sense, no need to change in that case.
|
This just went out in the dev digest - it can be merged in a day or 2 |
|
Merging as per @eileenmcnaughton 's comment |
|
Thanks all |
Overview
Give the custom CSS file a useful 'name' to make it easier to manipulate in the alterBundle hook.
Before
In the resources bundle, the custom CSS snippet has a 'name' attribute the same as the path - something like
http://example.org/sites/default/files/civicrm/css/custom.css?r=sdEtXAfter
In the resources bundle, the custom CSS snippet has a 'name' attribute of 'civicrm:css/custom.css'. This enables implementations of
hook_civicrm_alterBundle()to target the custom css file reliably.Technical Details
Nothing
Comments
This should not result in any changes other than implementations of
hook_civicrm_alterBundle()Has a test.