Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
ajax system
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
22 Sep 2013 at 17:03 UTC
Updated:
29 Jul 2014 at 22:57 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
wim leersThis is inside test coverage, so this is guaranteed to work. Only the code style needs to be reviewed.
Comment #2
wim leersI found even more to be removed.
Comment #3
tstoecklerLooks good.
Comment #4
wim leersComment #5
wim leersThe last two hunks should set
#attachedon$form.Comment #6
cosmicdreams commentedFrom AjaxResponse->ajaxRender:
This should be handled as well.
Comment #7
wim leers#6: no, see #2073819-4: [META] Remove direct calls to drupal_add_css() — this issue is solely about removing
drupal_add_css()calls that are used to add assets. Not for any others, like these, which are essentially "let's see which CSS and JS were added to this page". They're a different problem space, one that we can only fix once all calls that add assets are gone! :)Comment #9
xjmComment #10
vijaycs85Re-roll + addressing #5
Comment #12
vijaycs85Locally test is fine. So 10: 2095335-ajax-10.patch queued for re-testing.
Comment #14
vijaycs85Sorry testbot, it was my fault...
Comment #15
wim leersWas going to RTBC this, but found three very small nitpicks. Fixed those. I don't think they should prevent me from RTBC'ing.
Comment #16
vijaycs85Changes in #15 looks good to me, so +1 for RTBC.
Just a question:
As per #5, addd this line. Can we re move drupal_render() and keep $form['#attached']? will that make any difference? or better way of doing?
Comment #17
wim leers#16: I tried doing that, but that caused the tests to fail. It'd be cleaner, so I'd much prefer that, but I couldn't get it to work (I really tried), and it doesn't make sense to hold this patch up on that, which is why I'm just pressing ahead. In any case it's a step forward.
Comment #18
alexpottCommitted ff4a8e9 and pushed to 8.x. Thanks!
Comment #19
wim leers