Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dragan Eror’s picture

Assigned: Unassigned » Dragan Eror
Issue tags: +drupaldevdays

Will check this one...

Dragan Eror’s picture

Assigned: Dragan Eror » Unassigned
sqndr’s picture

Issue tags: -drupaldevdays

Any thoughts on how to fix this one?

nod_’s picture

Status: Active » Needs review
FileSize
1.94 KB

Here is a patch, the functionality works but it needs tests.

Status: Needs review » Needs work

The last submitted patch, 4: core-color-library-css-2228745-4.patch, failed testing.

nod_’s picture

FileSize
1.95 KB
550 bytes
nod_’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: core-color-library-css-2228745-6.patch, failed testing.

sqndr’s picture

+++ b/core/modules/color/color.module
@@ -90,6 +90,33 @@ function color_css_alter(&$css) {
+  ¶

Gave me a whitespace error. New patch added.

@nod_: What kind of tests does it need? Manual testing or ... If I could help, give a shout! :)

nod_’s picture

Oh yeah feel free to take it up. Don't have much time for it.

The test needed is adding a new library to color_test_theme (in the color/tests/color_test folder), declare a css file from that library to the color module (in color.inc) and make sure that file gets rewritten properly.

sqndr’s picture

Assigned: Unassigned » sqndr

@nod_: Great, thanks. I'll try this out, assigning to myself.

RainbowArray’s picture

Issue tags: +Needs tests

Any progress on this, sqndr? It would be nice to get this in so we can get stylesheets out of info.yml and just have them in libraries.yml.

sqndr’s picture

Assigned: sqndr » Unassigned

@mdrummond: No progress so far. You can pick this up if you want.

undertext’s picture

Status: Needs work » Needs review
FileSize
4.43 KB

I have added a new library to color_test_theme and declared a css file from that library to the color module. It seems that current tests are checking file rewriting ( 'Make sure the color stylesheet is included in the content' assertion').
Also list_themes() function is deprecated now. So i use \Drupal::service('theme_handler')->listInfo() in patch.
Should we create a new issue about replacing list_themes() function occurrences ?

nod_’s picture

probably yes. Although I'm not certain what's the workflow those days.

m1r1k’s picture

Issue tags: +#ams2014contest
RainbowArray’s picture

Status: Needs review » Needs work

Part of the reason for this patch is so that theme info.yml files don't have stylesheet attachments inside them anymore. So it would be nice if this patch pulled those declarations out of info.yml. Alternatively that could be handled in a follow-up, but the todo points at this issue for that task.

Other things I spotted:

  1. +++ b/core/modules/color/color.module
    @@ -91,6 +91,36 @@ function color_css_alter(&$css) {
    +  global $theme_key;
    

    Is this the way this is being handled elsewhere? Using a global like this doesn't feel like a very d8 way to do this.

  2. +++ b/core/modules/color/src/Tests/ColorTest.php
    @@ -66,8 +66,8 @@ function setUp() {
    -        'scheme_color' => '#3b3b3b',
    ...
    +        'scheme_color' => '#3c3c3c',
    
    +++ b/core/modules/color/tests/modules/color_test/themes/color_test_theme/color/color.inc
    @@ -8,13 +8,25 @@
    +        'text' => '#3b3b3b',
    ...
    +        'text' => '#3c3c3c',
    
    +++ b/core/modules/color/tests/modules/color_test/themes/color_test_theme/css/colors.css
    @@ -0,0 +1,7 @@
    +    color: #3b3b3b;
    

    Why are some of these 3b3b3b and some 3c3c3c?

  3. +++ b/core/modules/color/tests/modules/color_test/themes/color_test_theme/color_test_theme.info.yml
    @@ -3,3 +3,5 @@ type: theme
    \ No newline at end of file
    
    +++ b/core/modules/color/tests/modules/color_test/themes/color_test_theme/color_test_theme.libraries.yml
    @@ -0,0 +1,5 @@
    \ No newline at end of file
    
    +++ b/core/modules/color/tests/modules/color_test/themes/color_test_theme/css/colors.css
    @@ -0,0 +1,7 @@
    \ No newline at end of file
    

    Missing newlines at the ends of these files.

tim.plunkett’s picture

sqndr’s picture

Assigned: Unassigned » sqndr
Issue tags: +FUCK
sqndr’s picture

Assigned: sqndr » Unassigned
Status: Needs work » Needs review

Here's a patch - and an interdiff with #14.

sqndr’s picture

The last submitted patch, 9: core-color-library-css-2228745-9.patch, failed testing.

joelpittet’s picture

Issue tags: -FUCK +FUDK

Not sure if you meant that tag? :p

sqndr’s picture

Haha - brilliant. Sorry 'bout that :D

RainbowArray’s picture

+++ b/core/modules/color/src/Tests/ColorTest.php
@@ -66,8 +66,8 @@ protected function setUp() {
+        'scheme_color' => '#3c3c3c',

+++ b/core/modules/color/tests/modules/color_test/themes/color_test_theme/color/color.inc
@@ -8,13 +8,25 @@
+        'text' => '#3b3b3b',
...
+        'text' => '#3c3c3c',

We still have one for 3b3b3b and two for 3c3c3c. Maybe there is something going on there that I don't understand. But if not, can we get these consistent?

Part of the point of this is that it allows us to no longer have stylesheet declarations in a theme info.yml file. Guessing we will handle that in a separate issue? The @todo points to this issue for that.

andypost’s picture

+++ b/core/modules/color/color.module
@@ -93,6 +93,36 @@ function color_css_alter(&$css) {
+  $theme_key = \Drupal::theme()->getActiveTheme()->getName();
+  $themes = \Drupal::service('theme_handler')->listInfo();
+  $theme_libraries = $themes[$theme_key]->info['libraries'];

No reason to disturb theme_handler

$active_theme = \Drupal::theme()->getActiveTheme();
$theme_libraries = $active_theme->getLibraries();
Wim Leers’s picture

  1. RE: the first bit of #25, regarding the confusing colors: it took me trial & error and debugging to understand what's going on. I've changed the colors a bit to hopefully make it easier to understand what's going on. I doubt ColorTest worked as intended before sqndr's changes — it didn't actually test anything for color_test_theme — fortunately, it did test Bartik correctly.
  2. Like the last bit of #25 says, the original goal of this patch was to allow for CSS to be defined in a library, and the theme's info YML file to have a libraries key. That worked, but Color module didn't support that yet.
    So, restoring the second hunk from #4 to do that again, and things work wonderfully well for Bartik :)
  3. #26 fixed.
  4. The latest reroll of this patch lost some of the earlier hunks: see interdiff-lost-hunks.txt.

The test-only patch should have two failures: one for Bartik, one for the test theme.


It would be nice to get this in so we can get stylesheets out of info.yml and just have them in libraries.yml.

+1!

Wim Leers’s picture

Priority: Normal » Major
Issue tags: +Developer Experience, +TX (Themer Experience)

Bumping to major because this is a blocker to shipping with Bartik's assets in a libraries.yml file, and hence it prevents a best practice from being visible in Drupal 8's only front-end theme.

Status: Needs review » Needs work

The last submitted patch, 27: color-module-issue-#2228745-27-test-only-FAIL.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
nod_’s picture

Status: Needs review » Reviewed & tested by the community

Test looks good to me :D

Since the logic hasn't changed You could say wim RTBC'ed that part of the patch. Two halves makes one RTBC patch :p

Wim Leers’s picture

Heh :)

RainbowArray’s picture

+1 zillion to this.

Theme sanity, yay!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed ce7fefd and pushed to 8.0.x. Thanks!

  • alexpott committed ce7fefd on 8.0.x
    Issue #2228745 by Wim Leers, sqndr, nod_, undertext: Fixed Color module...

Status: Fixed » Closed (fixed)

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

Jeff Burnz’s picture

YesCT’s picture

there was only one issue (this one with Developer Experience tag. I think DX (Developer Experience) is the one being used mostly.