LGTM On Mon, Sep 13, 2010 at 12:26 PM, <b...@google.com> wrote:
> > http://gwt-code-reviews.appspot.com/857802/diff/3001/4001 > File > > samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/PersonEditorWorkflow.java > (right): > > http://gwt-code-reviews.appspot.com/857802/diff/3001/4001#newcode109 > > samples/dynatablerf/src/com/google/gwt/sample/dynatablerf/client/PersonEditorWorkflow.java:109: > if (!editorDriver.getUnconsumedEditorErrors().isEmpty()) { > On 2010/09/13 17:36:08, rjrjr wrote: > >> Add a hasErrors() method for convenience? >> > > Also, naming nit: wonder if it's worth drawing so much attention to >> > the > >> Unconumed thing. It's not like you give any access to the consumed >> > ones, right? > >> So just call it getErrors, and javadoc that consumed ones have >> > been...consumed? > > Done. > > > http://gwt-code-reviews.appspot.com/857802/diff/3001/4019 > File user/src/com/google/gwt/editor/client/ValueAwareEditor.java > (right): > > http://gwt-code-reviews.appspot.com/857802/diff/3001/4019#newcode40 > user/src/com/google/gwt/editor/client/ValueAwareEditor.java:40: * peered > with > On 2010/09/13 17:36:08, rjrjr wrote: > >> Called by the EditorDriver to set the value on the object the Editor >> > is peered > >> with. ("provide access to" sounds like a getter) >> > > Done. > > > http://gwt-code-reviews.appspot.com/857802/diff/3001/4023 > File > user/src/com/google/gwt/editor/client/impl/AbstractEditorDelegate.java > (right): > > http://gwt-code-reviews.appspot.com/857802/diff/3001/4023#newcode78 > user/src/com/google/gwt/editor/client/impl/AbstractEditorDelegate.java:78: > private static class SimpleError implements EditorError { > On 2010/09/13 17:36:08, rjrjr wrote: > >> Should be in its own file, no? >> > > Done. > > > http://gwt-code-reviews.appspot.com/857802/diff/3001/4028 > File user/src/com/google/gwt/editor/client/ui/EditorErrorPanel.java > (right): > > http://gwt-code-reviews.appspot.com/857802/diff/3001/4028#newcode82 > user/src/com/google/gwt/editor/client/ui/EditorErrorPanel.java:82: if > (widget instanceof Editor<?>) { > On 2010/09/13 17:36:08, rjrjr wrote: > >> Do you really need the <?> on instanceof checks? It's not like it >> > means > >> anything. >> > > I get raw type warnings if I don't. > > > http://gwt-code-reviews.appspot.com/857802/diff/3001/4028#newcode112 > user/src/com/google/gwt/editor/client/ui/EditorErrorPanel.java:112: > errorLabel.getStyle().setDisplay(Display.INLINE_BLOCK); > On 2010/09/13 17:36:08, rjrjr wrote: > >> In JS we would do display="". Does our enum have a similar NONE value, >> > to allow > >> the default display behavior? >> > > This panel is pretty terrible as a bit of actual UI kit and will need to > be revisited anyway before a GA release. > > > http://gwt-code-reviews.appspot.com/857802/show > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
