In short, the patch introduces following features:

  • Added fragments to "Back to Top" links. So there's no total page reload, rather it links to ?q=faq# and hence moves to top of page when clicked.
  • Added option(in settings) to choose from Ordered list(ol) or Unordered list(ul) for both Questions and Categories listings, wherever applies.
  • Added return false; to .click(). So there's no page reload, when the content is supposed to be shown(dynamically) on the same page.
  • Removed lots of not required t() uses. Like for node titles, node body/content, taxonomy term descriptions, faq description, and lots more. User input is neved t()ed and node content's translation is maintained using i18n module. You probably get translated strings when calling for them.
  • ksort() in Categories block was removed, to respect positioning by term weights.

Had also thought of changing theme_example(..) to theme('example', ..), but just noticed in CVS, that you have already done it. :) The attached patch was supposed to be against 2.2 version of faq module. However, it seems to work well on latest version in DRUPAL-5 branch. You just have some query changes, which are out of patch's way. :-) There were chances of more improvements to the JS file, but didn't look deep into it for now.

Please review and comment. Bashing and thrashing welcome.

CommentFileSizeAuthor
faq.patch40.68 KBGurpartap Singh

Comments

stella’s picture

stella commented
Assigned: Gurpartap Singh » stella
Status: Needs review » Fixed

Thanks for that! I've been meaning to do a review of the code for a while as too many features got added too quickly. If you want to work on the js file some more please do. I'm not that familiar with jquery yet.

I've added pretty much all the changes you suggested. The only change I made to your patch was to change the default for the category/question list ordering to unordered rather than ordered. This way it's in keeping with what has been released previously and it won't break anyone's existing installation.

I've also added in variable_del() calls, for the new variables you added, in the uninstall function in the faq.install file, and have added some disabling of form elements for the un/ordered selection list as they are only used in certain layouts. It might be a nice feature to have for questions/categories that aren't part of ul/ol lists, i.e. the dt ones and there might be a neater solution for numbering sub-categories too, but I'm not sure when I'll get to look at either of this items.

Thanks for the code review! It's really appreciated.

Cheers,
Stella

stella’s picture

stella commented

Changes have been added to CVS, will be included in the next release.

killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Fixed » Closed (fixed)
stella’s picture

stella commented

Released in faq-5.x-2.3.

Cheers,
Stella