I think that NavBar.java should be introduced rather than making it a
static nested classs. Many indentation and style nits, do with them what
you will.


http://gwt-code-reviews.appspot.com/97805/diff/1/4
File
samples/mail/src/com/google/gwt/sample/mail/client/AboutDialog.ui.xml
(right):

http://gwt-code-reviews.appspot.com/97805/diff/1/4#newcode13
Line 13: <g:DockPanel>
Why not DockLayoutPanel? Your sample is generating deprecation
warnings...

http://gwt-code-reviews.appspot.com/97805/diff/1/5
File
samples/mail/src/com/google/gwt/sample/mail/client/ContactPopup.ui.xml
(right):

http://gwt-code-reviews.appspot.com/97805/diff/1/5#newcode31
Line 31: <ui:image field='photo' src='default_photo.jpg'/>
Nit: move this above <ui:style> to be a little less confusing?

http://gwt-code-reviews.appspot.com/97805/diff/1/6
File samples/mail/src/com/google/gwt/sample/mail/client/MailList.java
(right):

http://gwt-code-reviews.appspot.com/97805/diff/1/6#newcode49
Line 49: static class NavBar extends Composite {
Why isn't this NavBar.java?

http://gwt-code-reviews.appspot.com/97805/diff/1/7
File samples/mail/src/com/google/gwt/sample/mail/client/MailList.ui.xml
(right):

http://gwt-code-reviews.appspot.com/97805/diff/1/7#newcode8
Line 8: border-left: 1px solid #999;
Nit: should just indent two more spaces inside rules.

http://gwt-code-reviews.appspot.com/97805/diff/1/7#newcode41
Line 41: <ui:style field='selectionStyle'
type='com.google.gwt.sample.mail.client.MailList.SelectionStyle'>
Nit: to avoid this kind of wrapping I'm in the habit of indenting four
more spaces:

<ui:style field='...'
     type='...'>
   .selectedRow {...

http://gwt-code-reviews.appspot.com/97805/diff/1/8
File samples/mail/src/com/google/gwt/sample/mail/client/NavBar.ui.xml
(right):

http://gwt-code-reviews.appspot.com/97805/diff/1/8#newcode13
Line 13: <g:Anchor styleName='{style.anchor}' ui:field='newerButton'
href='javascript:;'>&lt; newer</g:Anchor>
Nit: could wrap at href (indent four more spaces)

http://gwt-code-reviews.appspot.com/97805/diff/1/9
File samples/mail/src/com/google/gwt/sample/mail/client/Shortcuts.ui.xml
(right):

http://gwt-code-reviews.appspot.com/97805/diff/1/9#newcode14
Line 14: gwt-image: 'gradient';
Nit: inconsistent indentation here and below

http://gwt-code-reviews.appspot.com/97805/diff/1/9#newcode66
Line 66: <g:header size='3'>
We're being inconsistent with g:header, I think. In DisclosurePanel it
means "put HTML here" and you use g:customHeader for a widget, but here
it means "give me a widget." Seems like we need to iron that out for RC.
Doesn't gate this review, though.

In the meantime, you don't need an HTMLPanel here, a plain old HTML
would do.

http://gwt-code-reviews.appspot.com/97805/diff/1/10
File samples/mail/src/com/google/gwt/sample/mail/client/TopPanel.ui.xml
(right):

http://gwt-code-reviews.appspot.com/97805/diff/1/10#newcode25
Line 25: <ui:image field='logo' src='logo.png'/>
Nit: another spot where it would be better to declare the image first.

http://gwt-code-reviews.appspot.com/97805

--~--~---------~--~----~------------~-------~--~----~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~----------~----~----~----~------~----~------~--~---

Reply via email to