Move taxonomy.theme.css to Starterkit Theme and/or Claro

moving taxonomy css to classy :)

The code looks ok, we need some before/after screenshots.

Added novice tag.

There is needs tags so needs work

Since the drupal.taxonomy library contains both JavaScript and CSS, I don't think you can safely just remove the CSS. Rather than attaching the theme CSS in the template, we probably need to add it as a dependency to the existing module library. See the patch in #2489564: Move user.theme.css to Classy for an example of how that's done. This issue may need to wait on that patch to land, since the classy.theme file doesn't exist yet.

Add some update to patch #2 and add dependence like in #2489564: Move user.theme.css to Classy

Can anyone figure out how to get these classes to display? I think this might be legacy code as it was added in #193333: Add drag and drop to taxonomy

Shall we remove it? :-)

+++ b/core/themes/classy/classy.theme
@@ -0,0 +1,15 @@
+    if ($extension == 'taxonomy' && isset($libraries['drupal.taxonomy'])) {
+        $libraries['drupal.taxonomy']['dependencies'][] = 'classy/drupal.taxonomy';
+      }

If this issue is going to be worked on, the indentations of this alter hook is little bit wrong :)

How to get these classes to display:

• switch admin theme to Classy
• go to a taxonomy 'list terms' form
• this page should have enough terms to be paginated

I recommend removing the CSS. Attached is a screenshot.

Also, I fixed the indentations for classy.theme.

Rerolled.

It seems like we are adding the Classy library in the template and altering the library? Do we need to do both?

Thanks for working on the issue! Here's a quick review:

1. +++ b/core/themes/classy/classy.libraries.yml
   @@ -21,3 +21,10 @@ search.results:
   +    component:
   +      css/taxonomy/taxonomy.theme.css: {}
   

   We can add drupal.taxonomy as dependency for this

2. +++ b/core/themes/classy/classy.theme
   @@ -0,0 +1,15 @@
   +/**
   + * Implements hook_library_info_alter().
   + */
   +function classy_library_info_alter(&$libraries, $extension) {
   +  if ($extension == 'taxonomy' && isset($libraries['drupal.taxonomy'])) {
   +    $libraries['drupal.taxonomy']['dependencies'][] = 'classy/drupal.taxonomy';
   +  }
   +}
   

   This can be removed

After investigating, loading this CSS on the correct page is really hard. This is for the term listing page, not for the frontend. This design doesn't make any sense either. I think we should remove it. I've traced the code back to #193333: Add drag and drop to taxonomy which was committed in 2007.

This is a pretty sweet patch with a lot of deleting :-) This needs manual testing to prove I haven't broken any Javascript functionality.

Lewis, can you clarify. We need the table drag functionality. Are you saying the functionality is already provided by something else (which I would have assumed) so having it in taxonomy module is just duplicating that functionality, and that is why it can be removed?

@davidhernandez Thanks right. The tabledrag library is added elsewhere. The taxonomy library just added on top of it for the benefit of styling.

After applying the patch the taxonomy listing page still looks familiar and drag and drop is working fine.

So apparently Lewis did not break the JavaScript.

@g.oechsler yeah, Thanks for confirming !! looks fine for me too !!

Please find the attached screencast for the same.

We need sign off from a taxonomy module maintainer just to make sure we aren't removing something valuable here. Assigning to xjm.

What is the case for doing this? Is this 100% dead code? Tinkering with the term overview is risky; it has broken over and over again during the D8 cycle in different ways, and we have no JS automated testing. (Actually it may still be broken in some ways; I'd have to go issue hunting.)

Otherwise, this issue would not pass the beta evaluation. As a normal task, it would need to be usability, accessibility, etc., or something unfrozen. CSS is unfrozen, but JS isn't.

Thanks for flagging the issue for me. :)

Also, in terms of testing this manually. @Manjit.Singh, that video is a great start! To have full confidence we'd need to test the indenting functionality, dragging around full hierarchies, and more than 50 terms.

@xjm Thanks. Looks like we should reduce the scope of this issue back to just moving the CSS to Classy

Ok, so because this CSS is actually admin CSS we can't add it though the Classy taxonomy template. It should be added to Seven.

I have some screenshots of what this JS+CSS does. I think it's supposed to show that there is another page of terms by highlighting. This doesn't really make a lot of sense to me.

// When a row is swapped, keep previous and next page classes set.

You'll want to use devel generate to create enough terms to create a second page of terms.

And the patch

+++ b/core/modules/taxonomy/taxonomy.libraries.yml
--- a/core/themes/classy/classy.libraries.yml
+++ b/core/themes/classy/classy.libraries.yml

+++ b/core/themes/classy/classy.libraries.yml
@@ -31,3 +31,5 @@ search.results:
+    component:
+      css/taxonomy/taxonomy.theme.css: {}

Was this overlooked? The CSS file is being put in Seven, but there is still a library being added in Classy.

+++ b/core/themes/classy/templates/content/taxonomy-term.html.twig
@@ -23,6 +23,7 @@
+{{ attach_library('classy/drupal.taxonomy') }}

Same.

Also, does that then mean it is indeed out of scope for the voltron patch to move the module .theme css to classy?

@davidhernandez Do you mean in this issue? Then yes. This is incorrectly named CSS. It should of been taxonomy.admin.theme.css. I'll roll a new patch with the classy changes removed.

What I meant is that since this isn't moving anything to Classy it indeed does not need to be rolled into the voltron patch to move the remaining module .theme.css to Classy. This one will be worked on independently.

Correct, this should be rolled into #2566775: [Voltron patch] Move all remaining *.admin.theme.css to Seven instead

Following up on #25 regarding the gray and white highlighting: the reason that the first page is white and then subsequent terms are gray is to make it possible to see what's after the current page in the hierarchy and allow terms to be rearranged across pages. Otherwise it would be impossible to reorder or reparent terms across pages.

Reroll

tested manual and found no regressions

So I haven't tested this but I am fairly sure we need to do something special in Stable to ensure the taxonomy CSS is still loaded there because we are using libraries override to replace this file in Stable. Perhaps libraries extend?

:)

Seems like we have to reroll the patch file :)

Reroll of #35 for 8.1.x

My comment from #37 still applies here.

stable.info.yml has this:

  taxonomy/drupal.taxonomy:
    css:
      component:
        css/taxonomy.theme.css: css/taxonomy/taxonomy.theme.css

Which will stop working with the latest patch as well.

Is it good to extend the library into seven, Because i have not used it till now. would it be impact somewhere else if we will use that ?

Example of Extending libraries :

# Extend drupal.user: add assets from classy's user libraries.
libraries-extend:
  core/drupal.user: 
    - classy/user1
    - classy/user2

I think in this case the libraries-extend approach does make sense. Otherwise I think we'd need to attach Seven-specific libraries in core modules which wouldn't make sense.

I think need only remove css from core module core\modules\taxonomy, because this css already in core\themes\stable\css\taxonomy\taxonomy.theme.css.

Attach a screenshot.

Thanks @Chernous_dn , I have tested it manually and the file (taxonomy.theme.css) is in stable theme and remove from the core module. It looks good to me :) I am moving it to RTBC to give it a closure.

+++ b/core/modules/taxonomy/taxonomy.libraries.yml
@@ -2,9 +2,6 @@ drupal.taxonomy:
-  css:
-    component:
-      css/taxonomy.theme.css: {}

Doesn't this break Stable? We could empty out taxonomy.theme.css from core with a comment explaining why it's empty but removing this file from the library definition would (I think anyway) break Stable's library override.

@cottser taxonomy.theme.css file is already exist in stable theme atcore/themes/stable/css/taxonomy. Can't we change the stable library definition ? and remove css file from core taxonomy module.
Is it making any sense ?

What Cottser is referring to is that Stable uses a library override. It does not create its own library for this file. We need to test that removing a file that is overridden doesnt prevent the file in Stable from being used. I think it does, and if so, we can't remove it. Or we would have to come up with some other backwards compatibility. Maybe creating the library in Stable will work. Off the top of my head I'm thinking not.

For the IS update.

Also not sure I agree with removing the css for the themes. There are many themes out there that would have to update their css now.

Well, I think the underlying concern of the original issue was that the CSS provided for the Taxonomy module was perhaps coupled to the design of Seven, and so should not leak into other themes when the taxonomy was administered in those themes. Rescoping the issue to take into account Seven having been removed from core.

Here's what's in taxonomy.theme.css today:

.taxonomy-term-preview {
  background-color: #eee;
}
.taxonomy-term-divider-top {
  border-bottom: none;
}
.taxonomy-term-divider-bottom {
  border-top: 1px dotted #ccc;
}

Actually, better to do this probably: Do these three lines of CSS belong in the Taxonomy module, or elsewhere?

(If the FEFMs agree this issue is worth doing, then we can go back to NW for the IS update and for an approach that doesn't have anything to do with Seven.)

After applied patch #47 manually working fine in drupal version 10 and php version 8.1. Refer to the screenshot.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.