Converts many cases of theme('table') to '#theme' => 'table' in order that hook_page_alter() implementations may customize the output.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

dmitrig01’s picture

Why __FUNCTION__? Can't you just write the name of the function?

moshe weitzman’s picture

Status: Needs work » Needs review
FileSize
15.96 KB

Sure you could, but it one more thing that can go out of sync. __FUNCTON__ is a convention used by the drupal_static() system so I thought I would use it here. I don't care that though if people think it is too damn ugly.

New patch tries to fix the test failure.

Status: Needs review » Needs work

The last submitted patch failed testing.

moshe weitzman’s picture

Status: Needs work » Needs review
FileSize
14.57 KB

Hmm. Lets try again.

Status: Needs review » Needs work

The last submitted patch failed testing.

kika’s picture

Me likes it better without __FUNCTION__

moshe weitzman’s picture

Status: Needs work » Needs review
FileSize
15.01 KB

now without __FUNCTON__ and hopefully passing tests.

kika’s picture

Looks good to me, nice followup for reducing #markup = theme() in core.

One thing though:

-  $output = t('This table lists all the recorded votes for this poll. If anonymous users are allowed to vote, they will be identified by the IP address of the computer they used when they voted.');

[snip]

+  $build['poll_votes_table'] = array(
+    '#prefix' => t('This table lists all the recorded votes for this poll. If anonymous users are allowed to vote, they will be identified by the IP address of the computer they used when they voted.'),
+  );

Instead free-hanging description text we now tie it more closely to the respective table but somehow it still feels as an hack.
Table theme function supports "caption" parameter -- what about using that? (I do not know how it would render in different browsers -- needs testing)

So it will become

+  $build['poll_votes_table'] = array(
+    '#caption' => t('This table lists all the recorded votes for this poll. If anonymous users are allowed to vote, they will be identified by the IP address of the computer they used when they voted.'),
+  );
moshe weitzman’s picture

FileSize
23.96 KB

@kika. didn't look right in the first browser i tried - ffox 3.5. see attached. odd centering of text. perhaps that can be fixed with css, but then we are working too hard IMO.

kika’s picture

Yep, afaik caption tag is centered by default. Go figure the html 2.x design.

I'd say: keep it in #prefix for now. Followup with new issue "find all ocurrences of caption-able text in core + fix caption styling".

eaton’s picture

Status: Needs review » Reviewed & tested by the community

I'm giving a big +1 to this -- like so many other parts of D7, this pushes rendering as late into the cycle as possible where it's easier to cache/optimize/alter/etc.

A great demonstration of how the new tweaks to drupal_render() can improve stuff that was previously inaccessible.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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