Vojtech Szocs has posted comments on this change. Change subject: webadmin: UI for gluster volume snapshots ......................................................................
Patch Set 13: (3 comments) Some more small comments, otherwise looks good, please address Kanagaraj's comments too. http://gerrit.ovirt.org/#/c/35082/13/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/popup/gluster/GlusterVolumeSnapshotCreatePopupPresenterWidget.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/popup/gluster/GlusterVolumeSnapshotCreatePopupPresenterWidget.java: Line 19: Line 20: @Override Line 21: public void init(final GlusterVolumeSnapshotModel model) { Line 22: super.init(model); Line 23: model.getForceCreate().getEntityChangedEvent().addListener(new IEventListener<EventArgs>() { +1 for moving model listener registration into PresenterWidget =) Line 24: @Override Line 25: public void eventRaised(Event<? extends EventArgs> ev, Object sender, EventArgs args) { Line 26: getView().setForceLabelVisibility(model.getForceCreate().getEntity()); Line 27: } http://gerrit.ovirt.org/#/c/35082/13/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GlusterVolumeSnapshotCreatePopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GlusterVolumeSnapshotCreatePopupView.java: Line 104: Line 105: @Override Line 106: public void edit(final GlusterVolumeSnapshotModel object) { Line 107: driver.edit(object); Line 108: forceWarningLabel.setVisible(object.getForceCreate().getEntity()); We already have method for this, so to avoid duplicating code: setForceLabelVisibility(object.getForceCreate().getEntity()); Line 109: } Line 110: Line 111: @Override Line 112: public GlusterVolumeSnapshotModel flush() { http://gerrit.ovirt.org/#/c/35082/13/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GlusterVolumeSnapshotCreatePopupView.ui.xml File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GlusterVolumeSnapshotCreatePopupView.ui.xml: Line 49: <g:VerticalPanel addStyleNames="{style.panelStyle}"> Line 50: <g:TextArea ui:field="forceWarningLabel" addStyleNames="{style.forceWarningLabel}"/> Line 51: </g:VerticalPanel> Line 52: Line 53: </g:VerticalPanel> > Instead of using multiple nested Vertical Panels, you can use FlowPanel. In Indeed, we should avoid "classic" GWT layout widgets like VerticalPanel and HorizontalPanel, because they are based on HTML <table> which is generally considered as bad practice for HTML layouting. As Kanagaraj suggested, you can use FlowPanel, which is really simple <div> based widget container. If you ever need to do more complex HTML layouting: * preferred option is to use Bootstrap CSS grids (Greg from UX team can give more details) * if above is not possible, use GWT 2.x "layout" panels like DockLayoutPanel etc. Line 54: </d:content> Line 55: </d:SimpleDialogPanel> -- To view, visit http://gerrit.ovirt.org/35082 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I823580ecb127e48e075c437668bfb19ff8ec4467 Gerrit-PatchSet: 13 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: Kanagaraj M <[email protected]> Gerrit-Reviewer: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[email protected]> Gerrit-Reviewer: anmolbabu <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
