Posted by joachim on July 31, 2012 at 7:23pm
3 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | documentation |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | needs backport to D7, Novice |
Issue Summary
API page: http://api.drupal.org/api/drupal/includes!form.inc/function/theme_tableselect/7
Enter a descriptive title (above) relating to theme_tableselect, then describe the problem you have found:
This is missing some ; and ,
Comments
#1
The code is also wrong... in that the table produced is empty because there is no #header property.
Perhaps I should remove the Novice tag...?
#2
I am a novice, but I'll have a go ;) How about something like this?
#3
Thanks! This is a good start... Someone needs to test the code to verify that it actually works now.
Also, I would prefer to see a code style more like:
$options = array(array (
'object' => 'Treacle',
'colour' => 'brown',
'#attributes' => array ('class' => array('brown-row')),
),
...
(that would be more consistent with what we normally do in Drupal Core).
Please also use the American spelling "color" instead of "colour". And we don't normally leave a space between array and the opening (.
#4
Thanks for the review jhodgdon. Here's another patch incorporating your suggestions.
#5
Looks pretty good, formatting-wise anyway... Someone needs to test the code and verify that it works.
Also, doesn't this need a t() on the title and empty text?
+ * '#title' => 'Table Select',+ * '#header' => $header,
+ * '#options' => $options,
+ * '#empty' => 'Nothing to show',
#6
I have rerolled the patch with a few updates thanks to jhodgdon and I tested the code.
#7
Better!
I guess I must not have looked too carefully at the formatting on the previous patch. I think there are a couple of things that need fixing here:
- array(...) should not have a space between array and (
- I think we could do away with all of the blank lines in the code sample. They take up a lot of space and I don't think they add a lot of clarity.
I am also confused about this example. This is a table-select... maybe we could use something that is more realistic? What is this treacle and corkscrew meant to be? I don't even think most people in the world would know what "treacle" is -- I'm not really sure myself. Also, what is the empty text meant to be saying?
#8
Thanks for the review jhodgdon. I have implemented the requested changes.
- Fixed up the spacing issue between array and (
- Cleaned up the white spaces
- Made the content a bit more realistic
#9
#10
Much better, thanks! I was about to commit this patch, but I realized it pertains to the @param, so it should be included in the @param instead of where it is now in the main body of the function docs (which doesn't make much sense, since it lacks context).
#11
Yes that does make sense. I have moved the
@codebelow the@param.#12
The last submitted patch, form-codeexample-1706926-11.patch, failed testing.
#13
#11: form-codeexample-1706926-11.patch queued for re-testing.
#14
The blank line between the existing @param text and the new text needs to be removed, and the new text should be wrapped with the previous text. Thanks!
#15
Here's another patch with the requested changes.
#16
Thanks! The only thing is, these two lines should be wrapped together:
* table row's HTML attributes; see theme_table().+ * An example of per-row options:
#17
Sorry about that, I have wrapped the text onto a single line:
* table row's HTML attributes; see theme_table(). An example of per-row options:* @code
#18
Looks good! I'll get this committed shortly.
#19
Actually, that line needs to be wrapped at 80 characters. It goes over by a few currently.
#20
Re-rolled the patch and wrapped the line at 80 characters.
#21
This time I think I'll be able to commit it. Thanks!
#22
Thanks! Committed to 8.x and 7.x.
#23
Automatically closed -- issue fixed for 2 weeks with no activity.