Remove drupal_add_css() from colour.module – use #attached instead.

Meta issue: http://drupal.org/node/1839338

CommentFileSizeAuthor
#12 2010636-12-color_module.patch1.35 KBadammalone
#9 2010636-9.patch1.25 KBwim leers
#2 2010636-remove-drupal_add_.patch986 bytesmcjim

Comments

mcjim’s picture

mcjim commented
Title: Remove drupal_add_css() from color.module — use #attached » Remove drupal_add_css() and drupal_add_js() from color.module — use #attached
mcjim’s picture

mcjim commented
Status: Active » Needs review
StatusFileSize
new 2010636-remove-drupal_add_.patch986 bytes

There was a comment in the code to use drupal_add_library, but I've just use #attached.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Needs work

Please let's follow the best practice and use drupal_add_library().
Implement hook_library_info() and add the JS and CSS there. I can give you more specific instructions, in case you need any help.

mcjim’s picture

mcjim commented

Specific instructions would be welcome: the CSS/JS being added seems specific to the theme settings form rather than a general library? I may be misunderstanding something.

mcjim’s picture

mcjim commented
Status: Needs work » Needs review

This is an odd one for sure.

color_library_info() already adds preview.js. The code here is checking to see if there is a preview_js file in the theme and adding that instead of colour/preview.js…

The CSS added is from the theme, also, so neither of these really count as libraries.

So, while this solution might not be perfect, it does help get rid of another call to drupal_add_css/drupal_add_js for now (see #1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff)

Re-setting to needs review to let the test bot have a go, but feel free to change if there is a better approach.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community

Ahh, you're totally right. That approach should totally be improved in 1000 different ways, but that's not for this issue, and as it stands using a library is not possible/sensible in this case.

Thanks for sticking with this and setting me straight!

catch’s picture

catch
he/him
Primary language English
commented

#2: 2010636-remove-drupal_add_.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2010636-remove-drupal_add_.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » wim leers
Status: Needs work » Postponed
Issue tags: +D8 cacheability
StatusFileSize
new 2010636-9.patch1.25 KB

Straight reroll.

We can't test this because theme-specific settings are currently broken in D8. #2040037: The requested page "/admin/appearance/settings/bartik" could not be found after installation

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Postponed » Needs review

#9 is broken. The CSS and JS no longer ends up on the page. I strongly suspect the root cause for this are the recent changes to the (pre)process layer being removed/working differently and hence #attached assets in that phase no longer working… but at the same time #2004286: Defer calls to drupal_get_* functions until printed inside a template by adding a RenderWrapper class has not yet been committed, so I'm not entirely sure. Needs debugging.

NR because it *is* possible to test the patch, you just have to clear your caches after installing current D8 head freshly — see #2040037: The requested page "/admin/appearance/settings/bartik" could not be found after installation for details.

Status: Needs review » Needs work

The last submitted patch, 2010636-9.patch, failed testing.

adammalone’s picture

adammalone
Primary language English
Location San Francisco
commented
Status: Needs work » Needs review
StatusFileSize
new 2010636-12-color_module.patch1.35 KB

Tiny tiny rewrite to pass form by reference

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Works great — thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 749587e and pushed to 8.x. Thanks!

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