(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/forum/templates/forum-icon.html.twig
core/modules/forum/templates/forum-list.html.twig
core/modules/forum/templates/forum-submitted.html.twig
core/modules/forum/templates/forums.html.twig

Files: 
CommentFileSizeAuthor
#37 copy_forum_templates_to-2349683-36.patch4.64 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,015 pass(es). View
#36 interdiff-30to36.txt630 bytesdavidhernandez
#36 copy_forum_templates_to-2349683-36.txt4.64 KBdavidhernandez
#30 interdiff-29to30.txt672 bytesdavidhernandez
#30 copy_forum_templates_to-2349683-30.patch4.64 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,936 pass(es). View
#29 copy_forum_templates_to-2349683-29.patch4.55 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,925 pass(es). View
#27 copy_forum_templates_to-2349683-27.patch9.85 KBManuel Garcia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,956 pass(es). View
#25 interdiff.txt365 bytesManuel Garcia
#25 copy_forum_templates_to-2349683-25.patch3.55 KBManuel Garcia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,933 pass(es), 4 fail(s), and 1 exception(s). View
#20 no-submitted-class-start.png135.05 KBDickJohnson
#20 no-submitted-class.png165.03 KBDickJohnson
#19 copy_forum_templates_to-2349683-19.patch4.53 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,576 pass(es). View
#14 forum-patch-issue-stark.png188.4 KBemma.maria
#14 forum-after-patch.png142.13 KBemma.maria
#14 forum-before-patch.png121.24 KBemma.maria
#4 copy_forum_templates_to-2349683-4.patch4.81 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,569 pass(es). View
#1 classy-forum.diff4.78 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,762 pass(es), 4 fail(s), and 1 exception(s). View

Comments

mortendk’s picture

Assigned: Unassigned » mortendk
Status: Active » Needs review
FileSize
4.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,762 pass(es), 4 fail(s), and 1 exception(s). View

moved the classes out of core
Discussion of the classes - should be done as a follow up, so bikesheedding wont stop classy ;)

Status: Needs review » Needs work

The last submitted patch, 1: classy-forum.diff, failed testing.

davidhernandez’s picture

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

Morten you agreed to put them in one folder. I'm not sure why these patches are now all going in subfolders.

mortendk’s picture

Status: Needs work » Needs review
Issue tags: +cssbanana
FileSize
4.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,569 pass(es). View

cause i was stupid ;)

this patch have issues with test its using xpath so killing the ID's & classes gives us issues

Status: Needs review » Needs work

The last submitted patch, 4: copy_forum_templates_to-2349683-4.patch, failed testing.

mortendk’s picture

Issue tags: +drupalhagen
mortendk’s picture

Assigned: mortendk » Unassigned
danquah’s picture

Assigned: Unassigned » danquah
danquah’s picture

Assigned: danquah » Unassigned
davidhernandez’s picture

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.

Status: Needs work » Needs review

emma.maria’s picture

Reviewing this now.

emma.maria’s picture

Status: Needs review » Needs work
FileSize
121.24 KB
142.13 KB
188.4 KB

Module templates

The following look fine to me, they have the classes and IDs stripped out.

core/modules/forum/templates/forum-list.html.twig
core/modules/forum/templates/forum-submitted.html.twig
core/modules/forums.html.twig

Here is what Stark looks like before and after the patch.
 

 

 

 

My issues with this patch are....

  1. +++ b/core/modules/forum/templates/forum-icon.html.twig
    @@ -18,15 +18,9 @@
    -    <a id="new"></a>
    +    <a></a>
    

    Should we leave a completely empty "a" link wrapper here? Doesn't the ID provide a function as an anchor link for new? I would put this back in the module file.

  2.  

  3. +++ b/core/modules/forum/templates/forum-icon.html.twig
    @@ -18,15 +18,9 @@
    -  <span class="visually-hidden">{{ icon_title }}</span>
    +  <span>{{ icon_title }}</span>
    

    We need to keep visually-hidden classes they provide a function for accessibility.

  4.  

  5. +++ b/core/modules/forum/templates/forum-list.html.twig
    @@ -43,37 +43,35 @@
    -        {% for i in 1..forum.depth if forum.depth > 0 %}<div class="indent">{% endfor %}
    -          <div class="icon forum-status-{{ forum.icon_class }}" title="{{ forum.icon_title }}">
    -            <span class="visually-hidden">{{ forum.icon_title }}</span>
    +        {% for i in 1..forum.depth if forum.depth > 0 %}<div>{% endfor %}
    +          <div title="{{ forum.icon_title }}">
    +            <span>{{ forum.icon_title }}</span>
    

    again visually-hidden needs to be put back in the module file.

 
Otherwise the patch does not break any other functioning of the forum module.

Also the original files from the module have been added to the Classy theme and contain no changes.

emma.maria’s picture

Issue tags: +Novice

Tagging novice for the improvement work said in #14 on the patch in #4.
The work in #14 are small tasks of putting things back into template files that were removed by the patch.

mortendk’s picture

1. hmm do we consider this functionality ? - i can see that it could be ?

2. the visually-hidden is still a a class - its not prefixed as beein essential or with a specific "drupal-stuff" ala js- etc.
Ib core Were assuming absolutely nothing on the frontend. we Dont wanna break the chance for a theme to do completely new things in the future. So yes visually hidden or whatever else unless its absoluterly nessersary for functionality should be removed - its up to a themer to add that it in.

davidhernandez’s picture

The visually-hidden stuff is part of a group of classes used for hiding content and accessibility. We should keep those in the core template. It might make sense to have those renamed or handled in a different way, but that's not something we should try to solve here. That should be a global Drupal core issue.

mortendk’s picture

Issue tags: -drupalhagen

@david true - lets keep them here then take that on as a general issue later on, so we can attacch that to the behaviour of classy

mortendk’s picture

Status: Needs work » Needs review
FileSize
4.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,576 pass(es). View

added back the new id & visually-hidden classes

DickJohnson’s picture

Everything else seems to be okay, but:
1. In classy/templates/forum-submitted.html.twig we say:

{% if time %}
  <span class="submitted">{% trans %}By {{ author|passthrough }} {{ time }} ago{% endtrans %}</span>
{% else %}}</span> 

But I can't see the submitted class in DOM. With Classy installed and enabled:
classy

With Stark installed and enabled:
Stark

2. In every file-comment we have line like

 * @see template_preprocess_forum_list()

Do we need these in Classy?

DickJohnson’s picture

I can't see the class without the patch either, so it seems that either I'm doing something wrong or then we have a place for follow up issue. From my point of view this is RTBC.

mortendk’s picture

this should be followup issue, also i dont really understand the issue ;) but please create it

Manuel Garcia’s picture

Status: Needs review » Reviewed & tested by the community

The patch still applies cleanly, no need to reroll.

Code looks good. I went ahead and did some manual testing:

  1. Installed forum module
  2. Created a forum node in "General discussion"
  3. Verified that classes are still there, no visual regressions.

@DickJohnson that template file is used only on forum listings, not on the final forum node page. So this means it shows up under /forum, /forum/1 and so on. The class appears there fine, so no problems there.

This good to go imho.

davidhernandez’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/forum/templates/forum-icon.html.twig
@@ -18,13 +18,7 @@
-<div{{ attributes.addClass(classes) }}>

attributes should stay. Only remove the addClass part.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
3.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,933 pass(es), 4 fail(s), and 1 exception(s). View
365 bytes

Good catch @davidhernandez , here is the updated patch (based on #19).

Status: Needs review » Needs work

The last submitted patch, 25: copy_forum_templates_to-2349683-25.patch, failed testing.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
9.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,956 pass(es). View

Oops... forgot to add the new files...

davidhernandez’s picture

@Manuel, check your Git settings to make sure it is optimized for copying. See the handbook instructions here - https://www.drupal.org/documentation/git/configure

If optimized, Git will make the file copy one line, like Morten's patch in #19, where it looks like the following:

diff --git a/core/modules/forum/templates/forum-icon.html.twig b/core/themes/classy/templates/forum-icon.html.twig
similarity index 100%
copy from core/modules/forum/templates/forum-icon.html.twig
copy to core/themes/classy/templates/forum-icon.html.twig

This makes it easy to review and commit, because we can see the file is copied and not edited.

If it isn't working, try git diff -C.

mortendk’s picture

Assigned: Unassigned » mortendk
FileSize
4.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,925 pass(es). View

rerolled the patch added back in the attributes that was missed before, good catch :)

davidhernandez’s picture

Assigned: mortendk » Unassigned
FileSize
4.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,936 pass(es). View
672 bytes
+++ b/core/modules/forum/templates/forum-list.html.twig
@@ -43,11 +43,10 @@
+        colspan="4"
       {%- else -%}
-        class="forum"

This else is not needed since the class was removed. Uploading a fix.

Otherwise, everything looks fine. All the forum modules CSS is visual, and there is no JS.

Status: Needs review » Needs work

The last submitted patch, 30: copy_forum_templates_to-2349683-30.patch, failed testing.

mortendk’s picture

Status: Needs work » Needs review

huh is testbot drunk ? heres some coffe go test

Manuel Garcia’s picture

Status: Needs review » Reviewed & tested by the community

@davidhernandez thanks for useful link & info, done -=]

I've read through the patch, it looks (even more) beautiful. RTBC'ing

mortendk’s picture

@manuel im using sourcetree it makes it all work for me

davidhernandez’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.64 KB
630 bytes

Fixing the space around the 'if'. Previously it would put a blank space when there was no colspan.

davidhernandez’s picture

FileSize
4.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,015 pass(es). View

Oops. Named that .txt for some reason. Same interdiff applies.

mortendk’s picture

Status: Needs review » Reviewed & tested by the community

Patch works and white space cleaned up

css
took a look at the css as well, the css file should be renamed from forum.module to forum.theme.css as theres no essential css for the forum to work.
im adding this as a followup issue, to get the MAT css fixed / at least have note on it.
#2421099: forum module css file follow MAT naming

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Theme changes are not blocked by beta. Committed e6e3034 and pushed to 8.0.x. Thanks!

  • alexpott committed e6e3034 on 8.0.x
    Issue #2349683 by davidhernandez, mortendk, Manuel Garcia, emma.maria,...

Status: Fixed » Closed (fixed)

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