SGTM On Mon, Aug 8, 2011 at 7:15 PM, <jlaba...@google.com> wrote:
> Ray and I hashed out the names a bit. I don't mind shortening > CellTableHeaderCreator to HeaderCreator. I don't think it'll cause any > conflicts down the road. > > @rjrjr - any objections to the shorter name? > > > > http://gwt-code-reviews.**appspot.com/1499808/diff/1/** > samples/showcase/src/com/**google/gwt/sample/showcase/** > client/content/cell/**CwCustomDataGrid.java<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<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 { > On 2011/08/02 03:47:57, skybrian wrote: > >> 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 removed the "retirement benefits" row completely. We only need one > example of a spanning row, so the "birthday this month" is fine. > > > > 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. > This is a sample that is meant to illustrate what you can do with GWT. > We show source code for people who want to delve into it, but the > primary purpose of Showcase is to show off GWT, not to provide example > code. > > > http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** > user/src/com/google/gwt/user/**cellview/client/**AbstractCellTable.java<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<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); > On 2011/08/02 03:47:57, skybrian wrote: > >> Perhaps add: assert tag.size() == 5 >> > > Done. > > > http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** > user/src/com/google/gwt/user/**cellview/client/** > CellTableHeaderBuilder.java<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<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> { > On 2011/08/02 03:47:57, skybrian wrote: > >> How about just HeaderBuilder? >> > > Its specific to CellTable/DataGrid. Other CellWidgets might have a > different header builder. Ray and I came up with > CellTableHeaderCreator. > > > http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** > user/src/com/google/gwt/user/**cellview/client/** > CellTableHeaderBuilder.java#**newcode35<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. > On 2011/08/02 03:47:57, skybrian wrote: > >> "(or footer)" >> > > Done. > > > http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** > user/src/com/google/gwt/user/**cellview/client/** > CellTableHeaderBuilder.java#**newcode44<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> { > On 2011/08/02 03:47:57, skybrian wrote: > >> 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. >> > > Ray and I spent some time hashing out names, and we settled on > CellTableHeaderCreator and Helper (instead of Utility). I don't like > Callback because it isn't used in response to an event or an action, its > just a helper class used to build the table. > > > http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** > user/src/com/google/gwt/user/**cellview/client/** > CellTableHeaderBuilder.java#**newcode47<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 > On 2011/08/02 03:47:57, skybrian wrote: > >> "Enables column-specific event handling for the specified element." >> > > Done. > > > http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** > user/src/com/google/gwt/user/**cellview/client/** > CellTableHeaderBuilder.java#**newcode52<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. > On 2011/08/02 03:47:57, skybrian wrote: > >> 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}." >> > > Done. > > > http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** > user/src/com/google/gwt/user/**cellview/client/** > CellTableHeaderBuilder.java#**newcode58<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); > On 2011/08/02 03:47:57, skybrian wrote: > >> Perhaps this should be named enableColumnHandlers? >> > > > Done. > > I like that better. > > > http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** > user/src/com/google/gwt/user/**cellview/client/** > CellTableHeaderBuilder.java#**newcode72<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 > On 2011/08/02 03:47:57, skybrian wrote: > >> Is this true? >> "This will normally a cell in a row returned by startRow()." >> > > Done. > > > http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** > user/src/com/google/gwt/user/**cellview/client/** > CellTableHeaderBuilder.java#**newcode75<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, > On 2011/08/02 03:47:57, skybrian wrote: > >> 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".) >> > > Done. > > > http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** > user/src/com/google/gwt/user/**cellview/client/** > CellTableHeaderBuilder.java#**newcode79<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. > On 2011/08/02 03:47:57, skybrian wrote: > >> Should this be something like: >> > > "Adds a header (or footer) row to the table, below any rows previously >> > added." > > Done. > > > http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** > user/src/com/google/gwt/user/**cellview/client/** > DefaultCellTableBuilder.java<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<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. > On 2011/08/02 03:47:57, skybrian wrote: > >> "Build" >> > > Done. > > > http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** > user/src/com/google/gwt/user/**cellview/client/** > DefaultCellTableHeaderBuilder.**java<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<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 > On 2011/08/02 03:47:57, skybrian wrote: > >> What is this an index into? >> > > Done. > > Specified that the index is the column index of the header > > > http://gwt-code-reviews.**appspot.com/1499808/diff/6001/** > user/src/com/google/gwt/user/**cellview/client/** > DefaultCellTableHeaderBuilder.**java#newcode235<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, > On 2011/08/02 03:47:57, skybrian wrote: > >> Could we make the builder the first argument? >> > > Done. > > > http://gwt-code-reviews.**appspot.com/1499808/<http://gwt-code-reviews.appspot.com/1499808/> > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
