simpletests for sticky tableheaders

Assigning api changes to 7.x. Best way to change this would to add a parameter to theme_table.

I added a quick patch that allows this. I don't know if the variable could be named better or another solution is preferred but this one is pretty straight forward and works as expected.

Subscribe ... I have needed this before. Seems like a reasonable approach to me.

It seems the line numbers are updated. I couldn't really find another difference why the patch would fail.

Attached is a new version against latest HEAD.

Silly me, forgot to attach the patch.

*Sigh*
08|11:35:34… jsomers: can anyone tell me why the testbot flagged http://drupal.org/files/issues/jsomers_336475_sticky_6.patch as Invalid PHP Syntax?
08|11:36:06… DamZ: jsomers: that's a glitch, please resubmit

I don't know why the patch is still rejected. The code works on my test site and I nor any of my colleagues can't seem to find anything out of the ordinary.

I came to the conclusion that the previous patches were using Windows line endings and not Unix line endings. Attached patch should fix that and, I hope, will work as expected.

We don't usually munge words together, so this should either be $sticky_header or $sticky, seems reasonable though. Would be good if the unrelated whitespace changes were removed from the patch.

Altered with suggestions described in #13. I opted to use the variable $sticky over $sticky_header primarily because it's shorter but since the class name itself is sticky-enabled I made the assumption it won't lead to any confusion.

The whitespace stuff seemed to be caused by a configuration setting of my IDE which removes trailing white-spaces.

Updated patch.

Tiny useful proper. RTBC.

Excellent! Committed to HEAD. :)

Please document this as an API change in http://drupal.org/node/224333.

Added documentation: http://drupal.org/node/224333#sticky_headers

Please let me know if it's insufficient or you had something else in mind.

Looks great. Thanks, j.somers!

there was a duplicate issue of this one, here http://drupal.org/node/415024. On the plus side, the solutions posted in both issues as exactly the same :) In the other issue we also added some simpletests, so I've rerolled those as a patch against the latest HEAD (which includes the fixes above).

Added a unit simpletest to check that tableheader.js and the class 'sticky-enabled' are handled properly when then flag is set or not.

Possible additional tests:

* when there are no table headers, check to make sure tableheader.js+class is not added.
* check 'admin/build/permissions' page to make sure tableheader.js is applied here (browser test, instead of just unit).

I don't write a lot of tests, so any feedback is appreciated. Thanks!

/me slaps self in head. How on earth could I forget simpletests on an API change?! Slacker of the year. :)

Apart from the minor detail of the comments needing to end in a period, and the "+ * Unit tests for theme functions" should probably say "Unit tests for theme_table()." this actually looks pretty good to me! I'll leave it as needs review for someone else to eyeball.

haha, thanks webchick :) rerolled with your improvements.

Patch Review of #23

I reviewed the tests for Karl's latest patch, and they seem to do exactly what's required. It's testing for the presence of tableheader.js (which makes those table headers "sticky" in the first place) or the lack therof, so both cases are covered.

I did reroll it with some whitespace removed, a couple of wording changes in the assertTrue success strings, and a comment change at the top of the file to reflect that this is no longer just handling the Theme API tests. I also moved the testThemeTableStickyHeaders() function above the testThemeTableNoStickyHeaders() function since logically the former is the default state. No functional code was changed in this reroll.

Simpletest Results
When running all Theme tests: 13 passes, 0 fails, and 0 exceptions

Patch Review Summary
This one is RTBC, and goes perfectly with the patch that's already been committed for this issue.

woah, that's odd. Line 91 of theme.test is failing at "tableheader.js was not included because $sticky = FALSE."

UPDATE: It looks like drupal_add_js is retaining it's settings between the two functions, because when I put them back where they were, the tests pass again. This should probably be turned into a browser-based test instead, to be really definite.

Naw, that's too much work. We'll just reset the contents of $scripts after each function finishes.

+    drupal_add_js(NULL, $options['type'] = 'reset');

...looks odd. Why not simply array('type' => 'reset')?

yeah, that's a slightly odd construction. changed to array('type' => 'reset').

Additionally:

+    $this->assertTrue(strpos($output, 'sticky-enabled'), t('Table has a class of sticky-enabled when $sticky = TRUE.'));

Why not simply $this->assertRaw() instead of performing strpos() manually?

assertRaw() won't work for unit tests as far as I know.

fixed missing close paren.

#426906: Static Caching API for drupal_add_js/css got committed and removed the option 'reset' from drupal_add_js (and _css), so switched over to use the new drupal_static_reset('drupal_add_js')

also more informative title.

getInfo() needs to be a static method now:

public static function getInfo()

Also we only have drupalWebTestCase() at the moment, assertRaw() works for all tests.

ok, here's a version that just uses assertRaw() and a public static getInfo().

Would it be difficult to backport this change to 6.x? I am running into a situation where I can hardly even load the permissions page because the sticky tableheader causes the browser to lock up. It seems like tableheader.js doesn't perform well when there are hundreds of rows and a dozen or so columns in the table.

rerolled against HEAD.

@Evan: the code is easy to port back to 6, but it only makes the js file optional when calling theme_table(). You'd have to modify the code that generates the permissions page in some other way to make use of this patch.

@kscheirer: Ok, good to know. I think I could form_alter() the permissions page generating code to use the new theme_table() argument, but I have to actually *have* a new theme_table() argument first.

I should've spotted this the first time around, but please leave the @file declaration as is - it's not related to these tests and those are standard across core. Apart from that nitpick this is ready to go.

here ya go catch

Lovely.

Committed to CVS HEAD.