Task description
This task is to modify the FAQ module so that it uses theme templates instead of 4 of its theme functions.

Modules in Drupal have the ability to allow their HTML presentation to be overridden by the theme layer. This gives a huge amount of flexibility and allows greater customisation of Drupal sites. In Drupal 5, "theme" functions were used to contain chunks of HTML output which could be overridden. This method is still supported in Drupal 6 but there is now an alternative and better way of implementing it using template files.

The FAQ module has a number of theme functions. To complete this task, you need to create a template file (.tpl.php) and a corresponding preprocessor function for each of the theme functions listed below. See the resources section for more information, particularly "Using the theme layer in Drupal 6". Four functions in the FAQ module have already been ported to use template files, so it is advisable to use these as references.

Theme functions:

* theme_faq_category_new_page()
* theme_faq_category_hide_answer()
* theme_faq_category_questions_inline()
* theme_faq_category_questions_top()

The final deliverable will be a patch against the 6.x version of the FAQ module posted to the issue queue that has been reviewed and marked "Ready to be Committed".

Resources

Primary contact
Stella Power (http://drupal.org/user/66894 - snpower)

CommentFileSizeAuthor
#7 faq_template_category5.patch66.14 KBmwrochna
#4 faq_template_category3.patch14.83 KBmwrochna

Comments

stella’s picture

stella commented
Title: GHOP #??: Modify D6 FAQ module to use theme templates » GHOP #123: Modify D6 FAQ module to use theme templates

http://code.google.com/p/google-highly-open-participation-drupal/issues/...

add1sun’s picture

add1sun commented

Claimed by cwgordon7.

stella’s picture

stella commented

and unclaimed.

mwrochna’s picture

mwrochna commented
Assigned: Unassigned » mwrochna
Status: Active » Needs review
StatusFileSize
new faq_template_category3.patch14.83 KB

First template - faq-category-questions-inline - is ready for review.
Differences I know about:
a) Line 1488 (of new faq.module file) is $node = node_load($node->nid);. In other template_preprocess_faqs it was changed to $node_obj = node_load($node->nid);, and then $node_obj is only used for access checking in the next line. Should I do the same in faq-category templates? (like in line 1349)
b) the question_label was part of $node['link'] earlier. I changed it like in faq-questions-inline, it's outside the link, inside <strong>, so that it can be changed in the .tpl file. Is it ok? (line 103 of .tpl, 1515 of old .module)
c) the anwer_label earlier was concatenated with the $node['body'] before rendering through filters. Now the label is added later, in the .tpl, like in other templates. But this pulls the label outside of <p> tags, putting it in a new line, which IMHO doesn't look good. Maybe we should revert the hard-coded html in this case? (line 1525 of old .module, 1358 of new .module, 109 of .tpl).

stella’s picture

stella commented

Hi,

For questions a, b and c, I'd do the same as what's implemented in the existing templates. Initial review of the patch looks good, but will have to get back to you with comments later today/early tomorrow. Thanks for your help with this task.

Cheers,
Stella

stella’s picture

stella commented

Hi mwrochna,

I've tested your first patch and it works perfectly! Well done! I only spotted one coding style error in faq.module (using the Code Review module):

Line 162: Functions should be called with no spaces between the function name
  $type = node_get_types ('type', $node);

It's only a minor typo and I think it was there in the original version you patched against.

Looking forward to reviewing your next patch.

Cheers,
Stella

mwrochna’s picture

mwrochna commented
StatusFileSize
new faq_template_category5.patch66.14 KB

All 4 functions done.
Sub_terms' depths are now not always 1 (ex. line 988 $sub_term->depth = $term->depth + 1;). It shouldn't change anything in the default theme, but other themes may use it.
d) in Site configuration › FAQ Settings, the options 'Display category name for answers' and 'Group ...' are disabled even when the appropriate question layout is on. Should I file a bug or did I just forget something? (Do you have the same problem?)

e) category_questions_top returned two outputs (in an array): $output and $output_answers. I think the only way to make templates from them was to split the function. The problem is both parts use the sql $result. For now I just copied the $result in an ugly way. It would be enough to pass an array of nids to both templates, because the nodes' contents are queried for again anyway (when checking access rights, see a) ). Should I add loops fetching nids from result in lines 761, 992 and 1094?

f) category_question_inline, category_question_hide_answer and category_question_new_page are almost identical. Maybe it would be better to merge them into one template/one preprocessing function with more parameters?

stella’s picture

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

Hi mwrochna,

That's perfect, well done!

d) yes, that's a bug in the original version. Changing line 1856 like below fixes it.

-   $form['faq_display'] = array('#type' => 'value',
+   $form['faq_display'] = array('#type' => 'hidden',
     '#value' => variable_get('faq_display', 'questions_top'),
  );

e) Yes, that would be a better solution. However, this is the last day for claiming GHOP tasks and you have provided patch which works perfectly and without any code style errors, so I'm inclined to mark this RTBC.

f) I'd prefer if we left them as separate files so as not to overly complicate the templates for anyone overriding the theme functions.

I'm marking this RTBC. Don't forget to attach the patches to the GHOP task too. Thanks for all your hard work on this task and good luck with your next GHOP task.

Cheers,
Stella

stella’s picture

stella commented
Status: Reviewed & tested by the community » Fixed

Changes checked in and GHOP task updated as complete, so go grab another task before the deadline kicks in! Good luck and thanks again for all your hard work.

Cheers,
Stella

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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

stella’s picture

stella commented

Released in FAQ 6.x-1.3.

Cheers,
Stella