Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#995 closed enhancement (wontfix)

SortableTable doesn't preserve tr and td attributes

Reported by: sjs@… Owned by: Tom Trenka
Priority: high Milestone:
Component: Widgets Version: 0.3
Keywords: Cc: sjs@…
Blocked By: Blocking:

Description

If you have an id attribute (or other attribute) on a td, SortableTable? doesn't preserve it.

Below is a patch that copies tr's rather than creating them from scratch:

Index: src/widget/html/SortableTable.js
===================================================================
--- src/widget/html/SortableTable.js	(revision 4441)
+++ src/widget/html/SortableTable.js	(working copy)
@@ -263,6 +263,7 @@
 				continue;
 			}
 			var o={};	//	new data object.
+			o.__row__ = rows[i];
 			var cells=rows[i].getElementsByTagName("td");
 			for(var j=0; j<this.columns.length; j++){
 				var field=this.columns[j].getField();
@@ -343,7 +344,8 @@
 		//	build the table and pop it in.
 		while(body.childNodes.length>0) body.removeChild(body.childNodes[0]);
 		for(var i=0; i<data.length;i++){
-			var row=document.createElement("tr");
+			//var row=document.createElement("tr");
+			row = data[i].__row__.cloneNode(true);
 			dojo.html.disableSelection(row);
 			if (data[i][this.valueField]){
 				row.setAttribute("value",data[i][this.valueField]);
@@ -352,10 +354,16 @@
 				row.className=this.rowSelectedClass;
 				row.setAttribute("selected","true");
 			} else {
-				if(this.enableAlternateRows&&i%2==1){
-					row.className=this.rowAlternateClass;
+				if (this.enableAlternateRows) {
+					if(i%2==1){
+						row.className=this.rowAlternateClass;
+					}
+					else {
+						row.className="";
+					}
 				}
 			}
+            /*
 			for(var j=0;j<this.columns.length;j++){
 				var cell=document.createElement("td");
 				cell.setAttribute("align", this.columns[j].align);
@@ -383,6 +391,7 @@
 				}
 				row.appendChild(cell);
 			}
+            */
 			body.appendChild(row);
 			dojo.event.connect(row, "onclick", this, "onUISelect");
 		}

Change History (9)

comment:1 Changed 13 years ago by Tom Trenka

Milestone: 0.4
Type: defectenhancement

That was actually by design and not a bug or defect; the sortable table is meant to be a simple, data-displaying widget that takes a very simple data model and displays it.

I'm going to call this an enhancement and kill the milestone for now; I'm not sure what other widgets are in active development that may or may not supercede the SortableTable?.

comment:2 Changed 13 years ago by Tom Trenka

btw I can't even take a look at the included patch you put up there without knowing whether or not you've filed a CLA; if you can indicate whether or not you have, that'd be great.

comment:3 Changed 13 years ago by sjs@…

I sent in a CLA 6/9. My name is Stephen Smith, I live in Columbus, OH, and I filled in sjs@… for email address. Hopefully that is enough information to find it.

That was actually by design and not a bug or defect;

Yea, I figured. Someone asked about it on IRC the other day so I took a quick look and came up with that patch. It's a very simple patch. I don't think it breaks any functionality, but you are better to judge that one than I. No biggie if you don't want it.

comment:4 Changed 13 years ago by Tom Trenka

Hi Stephen, Thanks for letting me know about the CLA; part of the issue is that not all of us know who filed one, and who someone is based on an email address, so I make it a general rule to ask before looking.

Part of the issue with preserving attributes is that the sortable table right now uses other attributes to accompish some of their tasks (particularly on the tr elements), and so trying to preserve some and not others is kind of hard to do.

I will probably be trying at some point in the near future to make some major revisions to the widget; at that point I think this particular bug will probably be solved, we'll see though.

comment:5 Changed 13 years ago by sjs@…

Hi Tom. I wondered if anyone was even going to know that I had sent the CLA in even as I sent it in ;)

"Part of the issue with preserving attributes is that the sortable table right now uses other attributes to accompish some of their tasks (particularly on the tr elements)"

True.

"so trying to preserve some and not others is kind of hard to do."

Have you had a chance to look over the patch? I'm just copying the original tr and modifying as needed. "Doesn't preserve all" doesn't seem like an argument not to preserve some to me. Recreating tr's from scratch seems like too much work, and also like it would become a nightmare of special cases for special data types. I'm of the lazy programmer is a good programmer school though ;)

The main thing the patch might break is if there is anything important going on in the for loop that I commented out. I took another look at the for loop last night and came up with this bit of code to set css class on cells in the selected column:

var cells=row.getElementsByTagName("td");
if (cells.length==0) cells=row.getElementsByTagName("th");
cells[this.sortIndex].className = this.columnSelected;
if (this.oldIndex >= 0) cells[this.oldIndex].className = "";

which leaves only these two items from the original for loop unimplemented

  1. calling disableSelection() when sortType is __markup__
  2. some code to format dates

I didn't understand the purpose of those bits, so I havn't tried to implement them. I'd be willing to if you can clue me in though.

I can provide a new patch with the above code snippet included, if you like.

Cheers, Stephen

comment:6 Changed 13 years ago by Tom Trenka

Stephen,

I just got done writing a new, beefer version of the sortable table for a commercial project; some of that code will either make it's way into SortableTable? or become a new widget with enhanced features. Right now, this particular widget will only build from a JSON data source but I'm planning on parsing existing data as well. One of the main things about the newer version is that it doesn't destroy the table, it sorts in place, among other things...which will hopefully take care of quite a few bugs, including this one.

comment:7 Changed 13 years ago by dylan

Milestone: 0.4

Tom, does this mean we're going to ignore Stephen's patch, or is it worth merging? Please decide and resolve.

comment:8 Changed 13 years ago by Tom Trenka

Resolution: wontfix
Status: newclosed

I think we're going to ignore Stephen's patch for now (not that it isn't worth it); but I think that I would like to remove Sortable within a couple of minor version points (i.e. probably for 0.6 if not sooner) in favor of the FilteringTable? widget.

Closing it out.

comment:9 Changed 12 years ago by (none)

Milestone: 0.4

Milestone 0.4 deleted

Note: See TracTickets for help on using tickets.