Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
As contained in #950034: Using the same source for main/secondary with a custom menu doesn't work
Related issues
- Core: #80855: Add element #type table and merge tableselect/tabledrag into it
- #453400: "-wrapper" HTML ID not output for #type checkboxes/radios (checkboxes automatically get an HTML ID, but radios do not)
- #1248940-3: "Manage fields" screens : broken table layout (empty cells)
- #1260860: Rework language list admin user interface (#header)
Comment | File | Size | Author |
---|---|---|---|
#23 | edge.tableselect.23.patch | 7.12 KB | sun |
#8 | edge.tableselect.8.patch | 5.99 KB | sun |
#4 | edge.tableselect.4.patch | 5.36 KB | sun |
#3 | edge.tableselect.3.patch | 4.22 KB | sun |
#1 | edge-HEAD.table_.1.patch | 4.14 KB | sun |
Comments
Comment #1
sunThankfully, I already prepared that core patch to be fully self-documenting. :)
Comment #2
sunCommitted to HEAD, since I badly need this functionality for another project. Needs tests and documentation (i.e., d.o issue reference).
Comment #3
sunAttached patch extends the existing #type 'table' with tableselect support by merging in the corresponding #process and #theme code of #type 'tableselect' of Drupal core.
This means that #type 'table' now fully supports
- simple tables
- tables with checkboxes, radios
- tables with "Select all" in the header
- tables with tabledrag
Comment #4
sunForgot the #value_callback.
Comment #5
yched CreditAttribution: yched commentedNeato.
Note that tabledrag is a bit tricky though - because on failed validation, you want to redisplay the table in the updated order. Field UI is dealing with for its own tables in field_ui_table_pre_render().
Comment #6
sunI'm looking at field_ui.admin.inc, but can't find what you're referring to...?
But that's indeed nice, too. Looks like the thing you wrote for Field UI additionally handles
- tabledrag regions
- tabledrag parenting/indentations
and also, I've just realized that this current Edge code doesn't add a weight field to the table when tabledrag is attached... whereas all of this bears the question of how much automation is expected from a #type table in the first place.
So overall, lots of room for improvement + rethinking, but I guess it's wise to make progress in smaller steps. :)
Comment #7
yched CreditAttribution: yched commentedYup, field_ui_table_pre_render() builds the tree from parenting and weight values (
$row['parent_wrapper']['parent']['#value']
,$row['weight']['#value']
), and deduces rendering order. This relies on the the hardcoded presence of the 'parent' and 'weight' elements, though.Handling of region rows is more complex still (relies on PHP callbacks per 'row type' + JS code...), and is probably best left aside.
Comment #8
sunFixed #process function callback of MODULE_process_TYPE() clashes with theme process functions.
Comment #9
tstoecklerJust a plug to note that currently http://drupal.org/project/table exists. There's also http://drupal.org/project/elements. Since Table hasn't seen commits since last March, maybe at least contact @kkaefer to mark it obsolete. On the other hand, development could also move there, and Edge would simply introduce a dependency on Table. Either way, efforts should be consolidated.
Comment #10
tstoecklerAlso, now looking at the code, I saw that 'colspan' is currently not supported for a cell. I'll see if I can roll a patch, and open a new issue, if so.
Comment #11
sunWas that a RTBC for #8 ? ;) Still contains some @todos, but we can fix them along the way.
Comment #12
tstoecklerActually by 'the code' I meant HEAD, I just mentioned it here, because this is the AFAIK the (one-and-)only issue for #type table.
Comment #13
sunThanks for clarifying ;)
It's likely that we're going to fix that column attributes/handling problem with a new concept of
#wrapper_attributes
, which I already wanted to introduce for form elements in core (because it's impossible to set attributes on the wrapping DIV.form-item container).Comment #14
Dave ReidIf we're adding new FAPI elements, might you consider adding them to http://drupal.org/project/elements?
Comment #15
tstoecklerHere's my Dreditor review. I have yet to try out the code. Will do that, though, in the next couple days.
Not your fault, but:
You mean
$form_state['values']
right?You can read my mind... :) (The comment below that is explaining that the keys of
#default_value
are used, not the values).Typo: make -> makes
Is this naming consistent with somewhere in core? If not, maybe rename
#js_select
with#select_all
?The sentence doesn't make sense. or -> when?
Just popped into my mind, might make sense, might not: What about a
#title_key
attribute, which defaults to'title'
. That would be more (/as ?) flexible and remove the need for the code below (finding an element with#title
set).Is there any specific reason we're applying the top-level attributes to each checkbox/radio? There probably is, but I don't see it.
It seems as though checkboxes automatically get an HTML ID, but radios don't. If that's so, isn't that a core bug?
Powered by Dreditor.
Comment #16
tstoecklerNow I've tried it out. It's pretty awesome, because it's literally a one-line change in your code and you get a whole lot for it.
Two things:
1. The #element_validate callback checks that you have select at least one row. Why is that? I don't see a reason for that. Maybe if #required => TRUE or something...
2. If you don't key your table-row-elements with proper string-names, but just use an indexed array, the first value will be 0 (integer). So that means if the row is checked the value will be '0' (string) and if unchecked the value will be 0 (integer) (like all unchecked rows), which both cast as false to boolean. I don't know if we should do anything about that, but I wanted to mention it, because it will probably confuse people down the road.
Comment #17
tstoecklerFor reference, the last thing mentioned in #15 is: #453400: "-wrapper" HTML ID not output for #type checkboxes/radios
(Might be good to add a @todo or something in the code?)
Comment #18
tstoecklerEhh, actually that's a Drupal 6 issue. #17 is bogus.
Comment #20
Lars Toomre CreditAttribution: Lars Toomre commentedI have started working on two custom forms (in D6 for now) that I hope to contribute back to the Drupal community. The first allows selective pruning of the dblog table on possible combinations of time and count values. The second does the same for the aggregator tables.
@sun your code here and that of @yched in the field_ui module are great examples of integrating sortable forms into themeable tables with possible js support. Using these two examples (and possibly others), does it make sense to make an effort to add a new #table element to the FAPI for Drupal 8? If so, I am interested in contributing to the effort as this is an element I believe many contributor modules could well use.
Comment #21
yched CreditAttribution: yched commentedFWIW : considerations about 'empty cells' in #1248940-3: "Manage fields" screens : broken table layout
Comment #22
sunIn #1260860: Rework language list admin user interface I learned that we also need to do something about #header.
I.e., it's nice that you can position an injected/altered-in column like this:
but to do the same for #header...
Comment #23
sunAttached patch is the latest code, also incorporating all comments on this issue.
And FWIW, I just simply committed that for now.
Comment #24.0
sunUpdated issue summary.
Comment #24.1
sunUpdated issue summary.