http://gwt-code-reviews.appspot.com/1499808/diff/1/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java File samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java (right):
http://gwt-code-reviews.appspot.com/1499808/diff/1/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java#newcode82 samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java:82: public class CwCustomDataGrid extends ContentWidget { This example doesn't really hang together. On the one hand it's showing friends and birthdays (like a social app or a contacts database) and on the other it's showing who's eligible for retirement benefits (like an HR app). It seems like we should pick one or the other? Perhaps take out "retirement benefits" and replace that with something that makes more sense for a contacts database? I also wonder if we should simplify it or split into two or more examples where each demonstrates one thing. For someone reading documentation, 800 lines of sample code seems really big and I'd much prefer to read a smaller example. http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java File user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java (right): http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java#newcode493 user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java:493: rawHtml = rawHtml.substring(7, rawHtml.length() - 8); Perhaps add: assert tag.size() == 5 http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java File user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java (right): http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode32 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:32: public interface CellTableHeaderBuilder<T> { How about just HeaderBuilder? http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode35 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:35: * A utility for building the header. "(or footer)" http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode44 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:44: public abstract class Utility<T> { Hmm, why isn't this an interface? It seems like "Callbacks" would be a better name, although it looks like you're using "Utility" elsewhere. http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode47 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:47: * Associate a {@link Column} with the specified element. If a column is "Enables column-specific event handling for the specified element." http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode52 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:52: * The element builder must be in a state where an attribute can be added. Could be moved to the "builder" parameter. Also, we could say something about where to get the builder. "The builder should be a child element of a row returned by {@link #startRow}." http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode58 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:58: public abstract void associateColumn(ElementBuilderBase<?> builder, Column<T, ?> column); Perhaps this should be named enableColumnHandlers? http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode72 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:72: * @param builder the {@link ElementBuilderBase} to render into Is this true? "This will normally a cell in a row returned by startRow()." http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode75 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:75: public abstract <H> void renderHeader(ElementBuilderBase<?> builder, Context context, I don't know how this fits into your existing conventions, but perhaps rename "builder" to "out"? (For render methods I like to put the thing being rendered into first, and name it "out".) http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode79 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:79: * Add a row to the table. Should this be something like: "Adds a header (or footer) row to the table, below any rows previously added." http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/DefaultCellTableBuilder.java File user/src/com/google/gwt/user/cellview/client/DefaultCellTableBuilder.java (right): http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/DefaultCellTableBuilder.java#newcode114 user/src/com/google/gwt/user/cellview/client/DefaultCellTableBuilder.java:114: // Builder the cell. "Build" http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/DefaultCellTableHeaderBuilder.java File user/src/com/google/gwt/user/cellview/client/DefaultCellTableHeaderBuilder.java (right): http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/DefaultCellTableHeaderBuilder.java#newcode214 user/src/com/google/gwt/user/cellview/client/DefaultCellTableHeaderBuilder.java:214: * @param index the index What is this an index into? http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/DefaultCellTableHeaderBuilder.java#newcode235 user/src/com/google/gwt/user/cellview/client/DefaultCellTableHeaderBuilder.java:235: protected void renderHeader(Utility<T> utility, ElementBuilderBase<?> builder, Header<?> header, Could we make the builder the first argument? http://gwt-code-reviews.appspot.com/1499808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
