tabledrag.js doesn't play nice with nested tables
berenddeboer - November 4, 2008 - 03:17
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | javascript |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
Hi All,
If I have a nested table inside a draggable table I get the following error:
cell[0] is undefined
[Break on this error] if (cell[0].colSpan > 1) {Looking at the source, it seems to me the columnIndex and headerIndex calculation should be based in the immediate element of their parent, or not? So the correct selector would be:
var columnIndex = $('> td', cell.parent()).index(cell.get(0)) + 1;
var headerIndex = $('> td:not(:hidden)', cell.parent()).index(cell.get(0)) + 1;(I added the "> " part).
Anything I didn't think off? This seems to work so far.
#1
BTW, the line number where the error occurs is 130.
#2
I've attached a patch that even allows for nested dragged tables. Obviously this needs some intricate support on the server side as well.
#3
Let's fix first in D7 and then backport.
#4
The last submitted patch failed testing.
#5
This patch works for me mate. I've worked for 3 weeks on a module with the assumption I could make nested drag tables - you can imagine the rude shock I copped when I tried various ways to nest tables.
Using your patch I am able to achieve what I need using the simplest technique - a FAPI array element within the 'value' FAPI array element of one dragtable row.
The problem now I that I cannot distribute my module until this bug is fixed in D6 core, or if I hack together a 2nd JS file that will install with my module - but then I will need to rewrite other tabledrag functions, probably.
Is there a chance this can be added to a D6 release, and how can I help?
#6
sorry chx, I type slow
So - the patch failed testing - what is wrong with it?
#7
Please don't change the version. We first need to patch core and then we'll backport and apply if necessary. It also needs to pass testing before it can be moved to RTBC.
#8
Not my fault mate, looks like this forum has a concurrent update bug.
#9
I guess I typed slow too. :) It happens. It's not a bug it's a feature. :)
Attached patch for D7
#10
Son of a monkey I swear I switched that to CNR.
@danielb you can help by testing this works in d7. I haven't tested it I just took the code from before.
#11
Thanks, I will write a nested drag table in Drupal 7.
#12
I can confirm that prior to applying the patch in Drupal 7 I was experiencing the same problem I had in Drupal 6. After applying the patch the behaviour was as originally expected.
For completeness I have attached the module I hacked together for this review, it's a bit rough as I had some trouble with theme_form_element, and may have misused drupal_render() to get around it, but that is just my inexperience with Drupal 7.
#13
Would be great if we could document this a bit better in the code. It wasn't immediately obvious... Quick re-roll with some code comments?
#14
Something like "match immediate children of the parent element to allow nesting"? Or more detailed? Should it be put in all four blocks of changed code?
#15
Patch with added comments.
#16
#17
Committed to CVS HEAD. Thanks. Moving version to D6
#18
Can we use this anywhere in core? Where are folks thinking of using this? screenshot?
#19
I am not sure a need exists to do a nested table in core, but since people like me need it for contrib modules, and the original poster needed it for his website, it shouldn't be prevented by a design that assumes no nesting.
The concept of nesting and hierarchies pops up constantly in development of all sorts of things. My need is for a user to configure a list of things that will happen. Some of these things have a sublist of processes that have to be ordered too. One solution is to have a seperate page for each sublist - but when the sublist is not important enough to have its own page, a nested table seems like the most convenient option for an interface.
Do we need to create a new patch for D6 with the comments, or will it be right?
#20
A new patch will need to be created against D6
#21
This is the D6 version of the patch.
#22
Committed to Drupal 6, thanks.
#23
Automatically closed -- issue fixed for 2 weeks with no activity.