(First, verify that the preprocess changes have been made. #2322163: [meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates.)

  1. Copy the Twig templates from the core module's templates directory to Classy's templates directory. Include all templates, even ones without classes.
  2. Remove all classes from the core module's template. Remove all classes added with addClass and ones that are hard-coded in the template.
  • If there are classes that are required for basic functionality, discuss whether they should be kept.
  • If there is CSS from the module, or anywhere else, referring to the class, discuss removing it or moving it to Bartik&Seven. Do not move the CSS to Classy.

Twig Templates to Copy

core/modules/taxonomy/templates/taxonomy-term.html.twig

CommentFileSizeAuthor
#29 taxonomy-quickfix.patch2.93 KBlauriii
#28 taxonomy-quickfix.diff1.48 KBmortendk
#27 2349765-taxonomy-20.patch.txt2.41 KBmortendk
#20 2349765-taxonomy-20.patch2.41 KBderheap
#9 2349765-9.patch1.62 KBderheap
#7 2349765-7.patch1.66 KBderheap
#1 2349765-1.patch1.62 KBderheap

Comments

derheap’s picture

derheap commented
StatusFileSize
new 2349765-1.patch1.62 KB

Removed classes and ids.
Copied the template to classy/templates/taxonomy.

There are some lines of CSS in the module, but they seem to be needed by the taxonomy overview form, so I left them alone.

derheap’s picture

derheap commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: 2349765-1.patch, failed testing.

Status: Needs work » Needs review

derheap queued 1: 2349765-1.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: 2349765-1.patch, failed testing.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented 
+++ b/core/modules/taxonomy/templates/taxonomy-term.html.twig
@@ -14,11 +14,7 @@
  * - attributes: HTML attributes for the wrapper. The 'class' attribute
- *   contains the following classes by default:
- *   - taxonomy-term: The current template type, i.e. "theming hook".
- *   - vocabulary-[vocabulary-name]: The vocabulary that this term belongs to.
- *     For example, if the term belongs to a vocabulary called "Tags" then the
- *     class would be "vocabulary-tags".
+ *   contains no classes by default.

I don't think we need to comment that there are no classes by default.

Also, the blank line is not needed on line 30 before the div where the term id is removed.

derheap’s picture

derheap commented
Status: Needs work » Needs review
StatusFileSize
new 2349765-7.patch1.66 KB

Made the changes according to #6.
And we have to keep the attributes on the div ( <div{{ attributes}}>).
Otherwise RDF-Support (and maybe other modules) break.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented
Status: Needs review » Needs work

The templates should go into the root of the templates folder, not a subfolder.

derheap’s picture

derheap commented
Status: Needs work » Needs review
Issue tags: +cssbanana
StatusFileSize
new 2349765-9.patch1.62 KB

OK. Moved to the template to the root template folder.

Tagging cssbanana, as there are some lines of css, which might be needed for admin.
We should investigate in a followup.

mortendk’s picture

mortendk commented
Issue tags: +drupalhagen
xen’s picture

xen commented
Assigned: Unassigned » xen
xen’s picture

xen commented

New template file is properly used.

xen’s picture

xen commented
Assigned: xen » Unassigned
Status: Needs review » Reviewed & tested by the community
davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented
Status: Reviewed & tested by the community » Needs review

I'm universally setting all the phase 2 issues back to needs review, because we're missing some things. Please double-check if any removed classes are being used in javascript. It is best to test the affected template using Stark to make sure nothing is broken.

mortendk’s picture

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

no .js is effecting the taxonomy terms templates.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/taxonomy/templates/taxonomy-term.html.twig
@@ -13,12 +13,7 @@
- * - attributes: HTML attributes for the wrapper. The 'class' attribute
- *   contains the following classes by default:
- *   - taxonomy-term: The current template type, i.e. "theming hook".
- *   - vocabulary-[vocabulary-name]: The vocabulary that this term belongs to.
- *     For example, if the term belongs to a vocabulary called "Tags" then the
- *     class would be "vocabulary-tags".
+ * - attributes: HTML attributes for the wrapper.

This fix is also applicable to the template being copying to classy. Let's keep them in sync.

Apart from that I can confirm the the classes added here are not used in any css or js.

derheap’s picture

derheap commented

OK, I will fix in the classy template too.

Can I remove the css classes, as they are not used?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I don't think we should be removing the classes. But I'll leave that to the likes of @davidhernandez, @john_albin or @mortendk to make the call.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

@derheap, are you referring to the classes in the Classy copy of the template? If so, no, those classes stay. The only classes that would get removed are in the template kept in the taxonomy module.

derheap’s picture

derheap commented
Status: Needs work » Needs review
StatusFileSize
new 2349765-taxonomy-20.patch2.41 KB

Fixed classy template according to #16.

@davidhernandez
Yes, I was over ambitious. I left them in.

mortendk’s picture

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

looks good to me. and yes we fist begin to hammer on classes when they are in classy

alexpott’s picture

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

Committed 0ba69d7 and pushed to 8.0.x. Thanks!

This normal task was committed due the fact the banana consensus received per approval wrt to #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?

  • alexpott committed 0ba69d7 on 8.0.x 
    Issue #2349765 by derheap | davidhernandez: Copy taxonomy templates to...
dcrocks’s picture

dcrocks commented

Why wasn't taxonomy-term.html.twig placed in the 'templates' sub-directory of classy, as the others were?

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

There shouldn't be any templates going into subdirectories. We will address that later. See #2349559: [meta] Discuss the organization of subfolders in Classy

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented
Status: Fixed » Needs work
Issue tags: +Quick fix

Oh I see. That went into the root. That's a problem. We need a quick fix.

mortendk’s picture

mortendk commented
Status: Needs work » Needs review
StatusFileSize
new 2349765-taxonomy-20.patch.txt2.41 KB

quickfix moving the taxonomy-term.html.twig into /templates

mortendk’s picture

mortendk commented
StatusFileSize
new taxonomy-quickfix.diff1.48 KB

*sigh* lets try again with the correct file

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
StatusFileSize
new taxonomy-quickfix.patch2.93 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Fixed
Issue tags: -Quick fix

I'm no longer committing on-issue followups since there cause issue management problems. For example, this is now a bug and not a task. Please open a new issue to do the move.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Related issues: +#2365897: Move taxonomy-term.html.twig to templates folder in Classy

Status: Fixed » Closed (fixed)

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