Problem/Motivation

The design implemented in the file taxnonomy.theme.css no longer makes sense

Proposed resolution

Remove the markup, CSS, and JS.

Remaining tasks

  • Agree.
  • Patch

User interface changes

Some removed borders and background colors

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because coding standards
Issue priority Not critical because coding standards
Unfrozen changes Unfrozen because it only changes CSS

Comments

Manjit.Singh’s picture

Assigned: Unassigned » Manjit.Singh
Manjit.Singh’s picture

Component: forum.module » taxonomy.module
Assigned: Manjit.Singh » Unassigned
Status: Active » Needs review
FileSize
2.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,356 pass(es). View

moving taxonomy css to classy :)

LewisNyman’s picture

Issue tags: +Needs screenshots

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

sqndr’s picture

Issue summary: View changes
Issue tags: +Novice

Added novice tag.

lauriii’s picture

Status: Needs review » Needs work

There is needs tags so needs work

jstoller’s picture

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.

Chernous_dn’s picture

FileSize
3.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,894 pass(es). View

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

Chernous_dn’s picture

Status: Needs work » Needs review
LewisNyman’s picture

Status: Needs review » Needs work

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? :-)

lauriii’s picture

+++ 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 :)

cmanalansan’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.65 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,860 pass(es). View
646 bytes
35.35 KB

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.

LewisNyman’s picture

Issue tags: -Needs screenshots, -Novice
FileSize
2.11 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,672 pass(es). View

Rerolled.

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

lauriii’s picture

Status: Needs review » Needs work

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

LewisNyman’s picture

Title: move taxonomy.theme.css to classy » Remove taxonomy.theme.css, related markup, and javascript
Issue summary: View changes
Status: Needs work » Active
Issue tags: +JavaScript
Related issues: +#193333: Add drag and drop to taxonomy

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.

LewisNyman’s picture

Issue tags: +Needs manual testing
FileSize
4.75 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,673 pass(es). View

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

LewisNyman’s picture

Status: Active » Needs review
davidhernandez’s picture

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?

LewisNyman’s picture

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

g.oechsler’s picture

Issue tags: -Needs manual testing

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.

Manjit.Singh’s picture

FileSize
542.63 KB

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

Please find the attached screencast for the same.

LewisNyman’s picture

Assigned: Unassigned » xjm

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

xjm’s picture

Assigned: xjm » Unassigned
Status: Needs review » Needs work
Issue tags: +Needs beta evaluation

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. :)

xjm’s picture

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.

LewisNyman’s picture

Title: Remove taxonomy.theme.css, related markup, and javascript » move taxonomy.theme.css to classy

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

LewisNyman’s picture

Title: move taxonomy.theme.css to classy » move taxonomy.theme.css to Seven
Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.46 KB
827.66 KB
425.54 KB

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.

LewisNyman’s picture

FileSize
2.71 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,830 pass(es). View

And the patch

davidhernandez’s picture

Status: Needs review » Needs work
+++ 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.

davidhernandez’s picture

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

LewisNyman’s picture

Issue summary: View changes

@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.

LewisNyman’s picture

Status: Needs work » Needs review
Issue tags: -Needs beta evaluation
FileSize
835 bytes
1.9 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,607 pass(es). View
davidhernandez’s picture

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.

LewisNyman’s picture

Status: Needs review » Needs work

The last submitted patch, 30: move_taxonomy_theme_css-2489580-30.patch, failed testing.

xjm’s picture

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.

lauriii’s picture

Status: Needs work » Needs review
FileSize
2.13 KB

Reroll

andypost’s picture

Status: Needs review » Reviewed & tested by the community

tested manual and found no regressions

Cottser’s picture

Status: Reviewed & tested by the community » Needs work

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?

andriyun’s picture

Assigned: Unassigned » andriyun
Cottser’s picture

Title: move taxonomy.theme.css to Seven » Move taxonomy.theme.css to Seven
Issue tags: -classy

:)

Manjit.Singh’s picture

Version: 8.0.x-dev » 8.1.x-dev

Seems like we have to reroll the patch file :)

kostyashupenko’s picture

Status: Needs work » Needs review
FileSize
2.12 KB

Reroll of #35 for 8.1.x

andriyun’s picture

Assigned: andriyun » Unassigned
Cottser’s picture

Status: Needs review » Needs work

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.

Manjit.Singh’s picture

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
Cottser’s picture

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.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Chernous_dn’s picture

Status: Needs work » Needs review
FileSize
849 bytes

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.

Chernous_dn’s picture

Attach a screenshot.

Manjit.Singh’s picture

Status: Needs review » Reviewed & tested by the community

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.

Cottser’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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.

Manjit.Singh’s picture

@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 ?

davidhernandez’s picture

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.

Manjit.Singh’s picture

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.