Forum nodes and comments use the default node and comment templates. This makes it difficult to theme the forums so they look like forums.

Granted, this isn't the huge architectural challenge this module needs, but it is a small improvement than can be done now.

Files: 
CommentFileSizeAuthor
#4 templates-forum-nodes-comments-1285842-4.patch10.53 KBmmcdermott
PASSED: [[SimpleTest]]: [MySQL] 33,051 pass(es). View
#1 templates-forum-nodes-comments-1285842-1.patch1.63 KBmmcdermott
FAILED: [[SimpleTest]]: [MySQL] 33,072 pass(es), 0 fail(s), and 142 exception(es). View

Comments

mmcdermott’s picture

Status: Active » Needs review
FileSize
1.63 KB
FAILED: [[SimpleTest]]: [MySQL] 33,072 pass(es), 0 fail(s), and 142 exception(es). View

And here is a patch.

I haven't put much CSS in the core module, but I am working on some modifications to Bartik which will come in a separate patch.

Status: Needs review » Needs work

The last submitted patch, templates-forum-nodes-comments-1285842-1.patch, failed testing.

larowlan’s picture

+++ b/modules/forum/forum.css
@@ -48,3 +48,14 @@
+.node-type-forum .author-info {
+ float: left;
+ padding-right: 10px;
+}

I'm not a huge fan of adding a float in core css, this kind of stuff belongs in the css cleanup that's actively happening - splitting core css into separate files to indicate their contents (eg admin, theme etc)

+++ b/modules/forum/forum.css
@@ -48,3 +48,14 @@
+.node-type-forum .node .content {
+ overflow: hidden;
+}

This will be a gotcha for themes lower down the line, I don't like the idea of it in core.

+++ b/modules/forum/forum.module
@@ -80,6 +80,14 @@ function forum_theme() {
+    'node__forum' => array(
+      'template' => 'node--forum',
+      'variables' => array(),
+    ),

This isn't needed, the theme_hook_suggestions in template_preprocess_node cover it

+++ b/modules/forum/forum.module
@@ -1186,6 +1194,20 @@ function template_preprocess_forum_icon(&$variables) {
 /**
+ * Process variables to format forum topic nodes.
+ */
+function forum_preprocess_node(&$variables) {
+  $variables['theme_hook_suggestions'][] = 'node__' . $variables['node']->type;
+}
+

This already happens in node module

+++ b/modules/forum/forum.module
@@ -1186,6 +1194,20 @@ function template_preprocess_forum_icon(&$variables) {
+function forum_preprocess_comment(&$variables) {
+  $variables['theme_hook_suggestions'][] = 'comment__' . $variables['node']->type;
+}

This looks good but does it belong here or in template_preprocess_comment (so this flexibility is available for those without the forum module).

Powered by Dreditor.

mmcdermott’s picture

FileSize
10.53 KB
PASSED: [[SimpleTest]]: [MySQL] 33,051 pass(es). View

Thanks for your feedback! Unfortunately the patch did not include the node--forum.tpl.php and comment--forum.tpl.php that should have been attached.

The intent of this patch is to provide default templates for forum nodes and comments. It is true that themes can currently create these files. However, default versions are not provided by forum.module. This is a start at addressing the issue of Drupal forums not looking like forums.

I'm sure there needs to be some discussion about what the right CSS is to include with this. The overflow:hidden (now changed to overflow: auto) has the effect of keeping the left edge of the content div aligned, with the user attribution aligned to the left. Use Firebug to turn it off and you'll see what I mean :) (why this accomplishes this effect is a bit of a mystery!). Other suggestions for achieving this effect would be welcome.

One thing I'm unsure about is including all the commented sections with the variables available to node--forum.tpl.php and comment--forum.tpl.php. It's useful to have them there when you're overriding the file, but if anything changes in the main files there it could be hard to keep it updated.

mmcdermott’s picture

Status: Needs work » Needs review
Everett Zufelt’s picture

Status: Needs review » Needs work

I would think that we can just provide a reference in the comment to node.tpl.php for inherited theme variables.

We need to use HTML5 sectioning elements, try following the discussion at #1077602: Convert node.tpl.php to HTML5

+<div class="<?php print $classes; ?> clearfix"<?php print $attributes; ?>>

* Not sure if clearfix is also hard coded in node.tpl.php, but it should be added in $classes_array

+  <div class="content"<?php print $content_attributes; ?>>

* Class should be added in $variables[content_attributes_array][class]

+<div id="node-<?php print $node->nid; ?>" class="<?php print $classes; ?> clearfix"<?php print $attributes; ?>>

* If this is rendered twice on the page won't there be dup ids?

+  <div class="node-content">

* Class should be added in $variables[content_attributes_array][class]
mmcdermott’s picture

Hi Everett,

Thanks for your feedback! All of those problems you mentioned are also in the current version of node.tpl.php. So, I wouldn't want to change them in my file but not in the standard node template.

Thanks also for pointing out the issue about converting node.tpl.php to HTML5. It sounds like there is some ongoing discussion on that. Could we get this patch submitted first, to align with the current node.tpl.php, and then update it as well once the new node.tpl.php is finalized? I would be happy to do that.

Everett Zufelt’s picture

Issue tags: +html5

I don't really see a good reason to roll this twice. D8 release is months/years away, we have time. Tagging

Jacine’s picture

Title: Templates for Forum nodes and comments » Forum module should implement its own node and comment templates to make theming easier
Issue tags: -html5 +theme system cleanup

In an effort to get a better picture of issues remaining in the HTML5 Initiative, we are removing the "html5" tag from issues that are not directly HTML5-related. While this patch does need to be updated to use HTML5 version of the node template, the issue itself is not really about HTML5, it's a request for the Forum module to implement it's own node and comment templates. I'm changing the title to better reflect that, and tagging "theme system cleanup" so those working on that initiative can consider this.

Note: Any issues assigned to the "Markup, CSS and JavaScript" components will still be broadcast on the HTML5 Twitter feed so that interested parties are aware and can participate.

shanethehat’s picture

This would be a good thing to look at when the Twig conversions for Forum (#1898418: forum.module - Convert PHPTemplate templates to Twig) and Comment (#1898054: comment.module - Convert PHPTemplate templates to Twig) are committed.

jenlampton’s picture

Issue tags: +Twig

For the forum module's version of the comment-wrapper template in particular, I was suggesting the code look like this:

{% extends comment-wrapper.html.twig %}

{% codeblock %}
{% endcodeblock %}

The otiginal comment-wrapper template will look like this:

<section id="comments" class="{{ attributes.class }}"{{ attributes }}>
  {% codeblock %}
    {{ title_prefix }}
    <h2 class="title">{{ 'Comments'|t }}</h2>
    {{ title_suffix }}
  {% endcodeblock %}

  {{ comments }}

  {% if form %}
    <h2 class="title comment-form">{{ 'Add new comment'|t }}</h2>
    {{ form }}
  {% endif %}

</section>

This is a perfect use case for Twig template inheritance.

See that issue: #1783190: Create a comment-wrapper template in forum module that inherits from the comment-wrapper template in comment module.

c4rl’s picture

One concern I have with how Twig inheritance is proposed to work is that the original template must decide what the child template is going to potentially want to change. That is, if there's not a codeblock that is useful to you in the original, then you have to override the whole thing anyway.

So, I'm not sure this API really provides us with anything new. Traditional overriding of the template registry is something we're familiar with and provides more flexibility.

lauriii’s picture

Issue summary: View changes
Status: Needs work » Closed (cannot reproduce)

This is fixed by something else so closing as irrelevant