This function builds a table when it should use the theme function instead.

CommentFileSizeAuthor
#6 stats-table.patch2.41 KBZen
#5 statistics.module_4.patch2.13 KBdavidtrainer
#2 statistics.module.diff__0.txt1.91 KBdavidtrainer
statistics.module.diff_.txt1.91 KBdavidtrainer

Comments

killes@www.drop.org’s picture

killes@www.drop.org commented
Version: 4.7.x-dev » 6.x-dev
Status: Needs review » Needs work

The patch appears to use tabs but otherwise looks good. Should be first applied to HEAD and D5.

davidtrainer’s picture

davidtrainer commented
StatusFileSize
new statistics.module.diff__0.txt1.91 KB

Fixed - sorry about that.

davidtrainer’s picture

davidtrainer commented
Status: Needs work » Needs review
dries’s picture

dries commented

We usually write $rows[] = instead of array_push(). It would be slightly more consistent to use the former.

The issue with this patch (and its quite minor) is that it doesn't generate semantically correct HTML -- titles/headers are no longer th-ed. So from that point of view, this patch is a small regression, especially because it wouldn't be themed consistently.

Thoughts?

davidtrainer’s picture

davidtrainer commented
StatusFileSize
new statistics.module_4.patch2.13 KB

Updated, per Dries' comments

Zen’s picture

Zen commented
Category: bug » task
Priority: Minor » Normal
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new stats-table.patch2.41 KB

Wasn't applying; re-rolled. Also:

  • Sync with HEAD - url() now takes an options array.
  • Patch did not retain pre-existing comment.

No changes of importance made to Dave's patch. Setting to RTBC.

-K

killes@www.drop.org’s picture

killes@www.drop.org commented

Still works as advertized.

dries’s picture

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

Committed to CVS HEAD. Thanks.

Anonymous’s picture

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