Updated: Comment #N
Problem/Motivation
Main meta issue: #2073819: [META] Remove direct calls to drupal_add_css()
Proposed resolution
Add a hook_library_info() in the theme and use #attached/drupal_render() instead of drupal_add_js/css.
Uses libraries in the same way #1969916: Remove drupal_add_js/css from seven.theme
The reason we are using hook_library_info() in this issue because replace hook_library_info() by *.library.yml file is an active issue and we will continue to use hook_library_info until https://drupal.org/node/1996238 is resolved.
Remaining tasks
None
User interface changes
None
API changes
None
Related Issues
#1996238: Replace hook_library_info() by *.libraries.yml file
#1969916: Remove drupal_add_js/css from seven.theme
#2073819: [META] Remove direct calls to drupal_add_css()
Manual testing - #29 Done
Technical review - #28, #24Done
Main meta issue: #1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff
Original report by @typhonius
Comment | File | Size | Author |
---|---|---|---|
#29 | maintenance-before.png | 65.75 KB | rteijeiro |
#29 | maintenance-after.png | 65.54 KB | rteijeiro |
#26 | 2040269-26.patch | 1.7 KB | RunePhilosof |
#26 | interdiff-21-26.txt | 695 bytes | RunePhilosof |
#22 | interdiff-15-21.txt | 689 bytes | RunePhilosof |
Comments
Comment #1
adammalonePatch attached...
Comment #2
Wim Leers#1: 2040269_1_remove-drupal_add_css-from-bartik.patch queued for re-testing.
Comment #4
Wim Leers.
Comment #5
Tim Bozeman CreditAttribution: Tim Bozeman commentedComment #6
Tim Bozeman CreditAttribution: Tim Bozeman commentedI emulated #44.
Comment #7
star-szrThanks Tim! There's no 'Needs review' tag, removing the tag and setting the status to 'needs review' so that the patch gets tested.
Comment #8
star-szrAlso I think we should be including a comment to explain the approach, so CNW to add that. Comment can be copied from the other patch.
Comment #9
Tim Bozeman CreditAttribution: Tim Bozeman commentedI will add the comment.
Comment #10
Tim Bozeman CreditAttribution: Tim Bozeman commentedI copied the comment from patch #44 and included it here.
Comment #11
Tim Bozeman CreditAttribution: Tim Bozeman commentedComment #12
star-szrThe comment should be in bartik_preprocess_maintenance_page above the render array/drupal_render(), not in hook_library_info…
Comment #13
Tim Bozeman CreditAttribution: Tim Bozeman commentedComment #14
Tim Bozeman CreditAttribution: Tim Bozeman commentedThank you for walking me through these issues Cottser!
I moved the comment.
Comment #15
star-szrThanks for your work on this @Tim Bozeman! I wanted to RTBC this but it did not pass manual testing (No bartik/css/maintenance-page.css showed up on the maintenance page). This revised patch does include the file, the only change in the resulting markup is the weight of maintenance-page.css.
Using RenderWrapper based on similar work done in seven.theme for #2004286: Defer calls to drupal_get_* functions until printed inside a template by adding a RenderWrapper class, in this case the result is the same as moving drupal_get_css() after the drupal_render().
Before:
After:
Comment #16
Tim Bozeman CreditAttribution: Tim Bozeman commentedComment #17
Wim Leers#15: 2040269-15.patch queued for re-testing.
Comment #19
star-szrTagging for reroll.
Comment #20
RunePhilosof CreditAttribution: RunePhilosof commentedI'll reroll it
Comment #21
RunePhilosof CreditAttribution: RunePhilosof commentedComment #22
RunePhilosof CreditAttribution: RunePhilosof commentedInterdiff in unified format.
Comment #23
RunePhilosof CreditAttribution: RunePhilosof commentedIn #10 it was stated that the solution used is from https://drupal.org/node/1969916#comment-7797531.
In that issue catch commented https://drupal.org/node/1969916#comment-7797647 that the solution is only valid because it was the maintenance theme.
I don't know if the correction in #15 solved that, or if it isn't an issue anymore.
Comment #24
rteijeiro CreditAttribution: rteijeiro commentedPatch applies well and seems to be right so it's a RTBC for me ;)
Comment #26
RunePhilosof CreditAttribution: RunePhilosof commentedCatching up with core: Issue #2067017: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants moved VERSION to \Drupal::VERSION.
I can see that the question I raised in #23 was a bit off.
It is fine because it is the maintenance page theming (inserted page there).
Comment #27
RunePhilosof CreditAttribution: RunePhilosof commentedI did a manual testing with simplytest.me and found that the maintenance-page.css file is sent to the browser.
Comment #28
kgoel CreditAttribution: kgoel commentedPatch applied cleanly and looks good.
Comment #29
rteijeiro CreditAttribution: rteijeiro commentedOk, it seems the patch works well:
BEFORE
AFTER
Go RTBC, go!!
Comment #29.0
kgoel CreditAttribution: kgoel commentedUpdated issue summary.
Comment #29.1
kgoel CreditAttribution: kgoel commentedUpdated issue summary.
Comment #29.2
kgoel CreditAttribution: kgoel commentedUpdated issue summary.
Comment #29.3
kgoel CreditAttribution: kgoel commentedUpdated issue summary.
Comment #29.4
kgoel CreditAttribution: kgoel commentedUpdated issue summary.
Comment #29.5
kgoel CreditAttribution: kgoel commentedUpdated issue summary.
Comment #29.6
kgoel CreditAttribution: kgoel commentedUpdated issue summary.
Comment #29.7
rteijeiro CreditAttribution: rteijeiro commentedUpdated issue summary.
Comment #29.8
kgoel CreditAttribution: kgoel commentedUpdated issue summary.
Comment #29.9
kgoel CreditAttribution: kgoel commentedUpdated issue summary.
Comment #29.10
kgoel CreditAttribution: kgoel commentedUpdated issue summary.
Comment #30
alexpottCommitted ded8dc7 and pushed to 8.x. Thanks!
Comment #31.0
(not verified) CreditAttribution: commentedUpdated issue summary.