anmolbabu has posted comments on this change.

Change subject: webadmin : Introduce volume profile pdf Export option
......................................................................


Patch Set 6:

(3 comments)

http://gerrit.ovirt.org/#/c/28453/6/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/VolumeProfileStatisticsModel.java
File 
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/VolumeProfileStatisticsModel.java:

Line 292:         if (!isBrickProfileSelected) {
Line 293:             url += ";nfsStatistics=true";//$NON-NLS-1$
Line 294:         }
Line 295:         return url + ".pdf";//$NON-NLS-1$
Line 296:     }
> I personally would write this like this:
Done
Line 297: 
Line 298:     public String getProfileExportUrl() {
Line 299:         return profileExportUrl;
Line 300:     }


http://gerrit.ovirt.org/#/c/28453/6/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/VolumeProfileStatisticsPopupView.java
File 
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/VolumeProfileStatisticsPopupView.java:

Line 33: import com.google.gwt.uibinder.client.UiBinder;
Line 34: import com.google.gwt.uibinder.client.UiField;
Line 35: import com.google.gwt.user.client.ui.Anchor;
Line 36: import com.google.gwt.user.client.ui.Label;
Line 37: import com.google.gwt.user.client.ui.VerticalPanel;
> Please don't use vertical panels. Use flow panels instead.
Done
Line 38: import com.google.inject.Inject;
Line 39: 
Line 40: public class VolumeProfileStatisticsPopupView extends 
AbstractModelBoundPopupView<VolumeProfileStatisticsModel> implements 
VolumeProfileStatisticsPopupPresenterWidget.ViewDef {
Line 41: 


Line 129: 
Line 130:     @UiField
Line 131:     @Ignore
Line 132:     @WithElementId
Line 133:     VerticalPanel nfsProfileAnchorContainer;
> Instead of exposing the containers, and dynamically adding the anchors, I w
Done
Line 134: 
Line 135:     ApplicationConstants constants;
Line 136:     ApplicationResources resources;
Line 137: 


-- 
To view, visit http://gerrit.ovirt.org/28453
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9791208b327dbe3e3b4dacfb568f08e042e8b8c9
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: anmolbabu <anb...@redhat.com>
Gerrit-Reviewer: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com>
Gerrit-Reviewer: anmolbabu <anb...@redhat.com>
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to