Following up on #1812048-11: Build the exposed form using form API functions, Views should use drupal_add_library to attach CSS/JS to form elements.

CommentFileSizeAuthor
#18 2005616-18-use_attached_library.patch1.99 KBmikeker
#13 2005616-13-use_attached_library.patch1.98 KBmikeker
#10 2005616-9-use_attached_librarry.patch2.82 KBmikeker
#5 2005616-5-do-not-test.patch2.82 KBmikeker
#2 2005616-2-do-not-test.patch2.74 KBmikeker
#1 2005616-1.patch2.74 KBmikeker

Comments

mikeker’s picture

mikeker commented
Status: Active » Needs review
StatusFileSize
new 2005616-1.patch2.74 KB

The attached patch makes this change, however it requires the patch in #1812048-9: Build the exposed form using form API functions to be applied first.

mikeker’s picture

mikeker commented
StatusFileSize
new 2005616-2-do-not-test.patch2.74 KB

No need to make the testbot do unnecessary work...

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -442,7 +442,7 @@ public function __construct(ViewStorageInterface $storage) {
-    $this->element['#attached']['css'][] = drupal_get_path('module', 'views') . '/css/views.base.css';
+    drupal_add_library('views', 'views.base');

+++ b/core/modules/views/views.theme.incundefined
@@ -1054,7 +1054,7 @@ function template_preprocess_views_exposed_form(&$vars) {
-  @$form['#attached']['css'][] = drupal_get_path('module', 'views') . '/css/views.exposed_form.css';
+  drupal_add_library('views', 'views.exposed-form');

We do not want to use drupal_add_library at all.

What we can do is

$this->element['#attached']['library'][] = array('views', 'views.base');

+++ b/core/modules/views/views.moduleundefined
@@ -844,13 +844,18 @@ function views_hook_info() {
+    'css' => array("$path/css/views.base.css"),

@@ -865,13 +870,18 @@ function views_library_info() {
+    'css' => array("$path/css/views.exposed_form.css"),

Can you split this onto multiple lines please?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Title: Views should use drupal_add_library rather than #attached » Views should use ['#attached']['library'] rather than ['#attached']['css']
mikeker’s picture

mikeker commented
StatusFileSize
new 2005616-5-do-not-test.patch2.82 KB

Got it. Thanks for the clarification.

Edit: Also breaks css => array() into multiple lines, as requested.

Status: Needs review » Needs work

The last submitted patch, 2005616-1.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Thank you very much!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Why the do not test patches? Please let the test bot run through just for sanity.

mikeker’s picture

mikeker commented
Status: Needs work » Needs review
StatusFileSize
new 2005616-9-use_attached_librarry.patch2.82 KB

I'm pretty sure this patch will not apply until the patch in #1812048: Build the exposed form using form API functions is committed. Is there a better way to handle inter-related issues like this? Should I write this patch to only change the code already in 8.x and reroll #1812048: Build the exposed form using form API functions once this is checked in?

I figured issues with patches that didn't pass the testbot wouldn't get looked at by the powers-that-be so I added the do-not-test suffix. Regardless, let's see what the testbot says...

Status: Needs review » Needs work

The last submitted patch, 2005616-9-use_attached_librarry.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented

Oh damn, you are right.

Btw. this patch misses the new css file.

mikeker’s picture

mikeker commented
Status: Needs work » Needs review
StatusFileSize
new 2005616-13-use_attached_library.patch1.98 KB

OK, slightly different approach... This patch only updates the existing CSS files to use ['#attached']['library'] rather than ['#attached']['css']. So there's no longer a dependency on any other issues.

I'll deal with updating the patch in #1812048: Build the exposed form using form API functions if/when this patch is committed.

Apologies for the back and forth -- I should've done it this way from the beginning.

Status: Needs review » Needs work

The last submitted patch, 2005616-13-use_attached_library.patch, failed testing.

mikeker’s picture

mikeker commented
Status: Needs work » Needs review

#13: 2005616-13-use_attached_library.patch queued for re-testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +VDC, +PHPUnit Blocker

Adding some tags.

alexpott’s picture

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

views.base.css is now views.module.css

mikeker’s picture

mikeker commented
Status: Needs work » Needs review
StatusFileSize
new 2005616-18-use_attached_library.patch1.99 KB

Chasing master...

damiankloip’s picture

damiankloip commented
Status: Needs review » Reviewed & tested by the community

Nice, those changes look OK to me!

alexpott’s picture

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

Committed 9c173cf and pushed to 8.x. Thanks!

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