Greg Sheremeta has posted comments on this change. Change subject: userportal, webadmin: new look and feel based on PatternFly ......................................................................
Patch Set 40: (17 comments) http://gerrit.ovirt.org/#/c/24594/40/backend/manager/modules/docs/src/main/webapp/WEB-INF/no_lang.jsp File backend/manager/modules/docs/src/main/webapp/WEB-INF/no_lang.jsp: Line 14: </fmt:message> Line 15: </title> Line 16: <obrand:stylesheets /> Line 17: </head> Line 18: <body onload="pageLoaded()"> > pageLoaded function is defined in splash.js - do we need it here? Done Line 19: <div class="obrand_loginPageBackground"> Line 20: <a href="<obrand:messages key="obrand.common.vendor_url"/>" class="obrand_loginPageLogoImageLink"> Line 21: <span class="obrand_loginPageLogoImage"></span> Line 22: </a> http://gerrit.ovirt.org/#/c/24594/40/frontend/webadmin/modules/gwt-common/pom.xml File frontend/webadmin/modules/gwt-common/pom.xml: Line 11: <name>oVirt GWT UI common infrastructure</name> Line 12: <dependencies> Line 13: <!-- Google Web Toolkit dependencies --> Line 14: <dependency> Line 15: <groupId>org.gwtbootstrap3</groupId> > I suggest to add gwtbootstrap3 dependency via separate "section", for examp Done Line 16: <artifactId>gwtbootstrap3</artifactId> Line 17: <version>0.5</version> Line 18: <scope>provided</scope> Line 19: </dependency> Line 13: <!-- Google Web Toolkit dependencies --> Line 14: <dependency> Line 15: <groupId>org.gwtbootstrap3</groupId> Line 16: <artifactId>gwtbootstrap3</artifactId> Line 17: <version>0.5</version> > According to [1] the latest stable release is 0.6, should we try it? Done Line 18: <scope>provided</scope> Line 19: </dependency> Line 20: <dependency> Line 21: <groupId>com.google.gwt</groupId> http://gerrit.ovirt.org/#/c/24594/40/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/GwtCommon.gwt.xml File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/GwtCommon.gwt.xml: Line 6: <!-- Inherit the core Web Toolkit stuff --> Line 7: <inherits name="com.google.gwt.user.User" /> Line 8: <inherits name="com.google.gwt.inject.Inject" /> Line 9: Line 10: <inherits name="org.gwtbootstrap3.GwtBootstrap3NoTheme"/> > +1 ack Line 11: Line 12: <!-- Inherit GWTP MVP module with automatic EntryPoint support --> Line 13: <inherits name="com.gwtplatform.mvp.MvpWithEntryPoint" /> Line 14: Line 32: <generate-with class="org.ovirt.engine.ui.common.binding.ElementIdHandlerGenerator"> Line 33: <when-type-assignable class="org.ovirt.engine.ui.common.idhandler.ElementIdHandler" /> Line 34: </generate-with> Line 35: Line 36: <set-configuration-property name="CssResource.style" value="pretty" /> > This turns off obfuscation of CSS class names generated for GWT CssResource removed. just here for development purposes (so I can hunt down classes) Line 37: http://gerrit.ovirt.org/#/c/24594/40/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/presenter/AbstractLoginPresenterWidget.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/presenter/AbstractLoginPresenterWidget.java: Line 109: modelCommandInvoker.invokeDefaultCommand(); Line 110: } Line 111: })); Line 112: Line 113: registerHandler(getView().getLoginForm().addKeyPressHandler(new KeyPressHandler() { > +1 ack Line 114: @Override Line 115: public void onKeyPress(KeyPressEvent event) { Line 116: if (event.getNativeEvent().getKeyCode() == KeyCodes.KEY_ENTER) { Line 117: modelCommandInvoker.invokeDefaultCommand(); http://gerrit.ovirt.org/#/c/24594/40/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/AbstractValidatedWidgetWithLabel.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/AbstractValidatedWidgetWithLabel.java: Line 98: contentWidgetElement.setId(DOM.createUniqueId()); Line 99: } Line 100: Line 101: // Connect label with content widget for better accessibility Line 102: // updateLabelElementId(contentWidgetElement.getId()); > If we don't want this code, we can simply remove it. Done Line 103: } Line 104: Line 105: protected void updateLabelElementId(String elementId) { Line 106: // labelElement.setHtmlFor(elementId); Line 102: // updateLabelElementId(contentWidgetElement.getId()); Line 103: } Line 104: Line 105: protected void updateLabelElementId(String elementId) { Line 106: // labelElement.setHtmlFor(elementId); > If we don't want this code, we can simply remove it. Done Line 107: } Line 108: Line 109: public void setUsePatternFly(final boolean use) { Line 110: usePatternFly = use; Line 106: // labelElement.setHtmlFor(elementId); Line 107: } Line 108: Line 109: public void setUsePatternFly(final boolean use) { Line 110: usePatternFly = use; > Please use this.usePatternFly for better readability. Done Line 111: if (use) { Line 112: // set the style to the bootstrap / patternfly style Line 113: setContentWidgetStyleName(PatternflyConstants.FORM_CONTROL); Line 114: //Set the content width back to default. Line 111: if (use) { Line 112: // set the style to the bootstrap / patternfly style Line 113: setContentWidgetStyleName(PatternflyConstants.FORM_CONTROL); Line 114: //Set the content width back to default. Line 115: // contentWidget.asWidget().setWidth("75%"); //$NON-NLS-1$ > If we don't want this code, we can simply remove it. Done Line 116: addLabelStyleName("label col-sm-2 col-md-3 control-label"); //$NON-NLS-1$ Line 117: addContentWidgetContainerStyleName("col-sm-10 col-md-9"); //$NON-NLS-1$ Line 118: wrapperPanel.getElement().addClassName(PatternflyConstants.FORM_GROUP); Line 119: wrapperPanel.getElement().removeClassName(style.wrapper()); Line 140: Line 141: @Override Line 142: public void setElementId(String elementId) { Line 143: getContentWidgetElement().setId(elementId); Line 144: // updateLabelElementId(elementId); > If we don't want this code, we can simply remove it. Done Line 145: } Line 146: Line 147: @Override Line 148: protected Widget getValidatedWidget() { http://gerrit.ovirt.org/#/c/24594/40/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/PatternflyUiCommandButton.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/PatternflyUiCommandButton.java: Line 27: Line 28: @Override Line 29: public int setTabIndexes(int nextTabIndex) { Line 30: //TODO: Implement focusable on the button so we can set tab index on this. Line 31: return 0; > In this case, default no-op implementation should be: Done Line 32: } http://gerrit.ovirt.org/#/c/24594/40/frontend/webadmin/modules/userportal-gwtp/pom.xml File frontend/webadmin/modules/userportal-gwtp/pom.xml: Line 14: <!-- Google Web Toolkit dependencies --> Line 15: <dependency> Line 16: <groupId>org.gwtbootstrap3</groupId> Line 17: <artifactId>gwtbootstrap3</artifactId> Line 18: <version>0.5</version> > Please see my comment in gwt-common/pom.xml - I suggest to remove <version> Done Line 19: <scope>provided</scope> Line 20: </dependency> Line 21: <dependency> Line 22: <groupId>com.google.gwt</groupId> http://gerrit.ovirt.org/#/c/24594/40/frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/widget/tab/UserPortalMainTab.java File frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/widget/tab/UserPortalMainTab.java: Line 15: private float priority; Line 16: private boolean accessible; Line 17: private AbstractTabPanel tabPanel; Line 18: Line 19: HTMLPanel root; > Since this is not @UiField, please mark "root" field as private and final. Done Line 20: Line 21: @UiField Line 22: public Anchor hyperlink; Line 23: http://gerrit.ovirt.org/#/c/24594/40/frontend/webadmin/modules/userportal-gwtp/src/main/resources/org/ovirt/engine/ui/userportal/UserPortal.gwt.xml File frontend/webadmin/modules/userportal-gwtp/src/main/resources/org/ovirt/engine/ui/userportal/UserPortal.gwt.xml: Line 38: <extend-property name="locale" values="en_US,es_ES,fr_FR,ja_JP,pt_BR,zh_CN,de_DE,ko_KR" /> Line 39: <set-property name="locale" value="${gwt.locale}" /> Line 40: <set-property-fallback name="locale" value="en_US" /> Line 41: Line 42: <set-configuration-property name="CssResource.style" value="pretty" /> > UserPortal module inherits from GwtCommon module, so if GwtCommon module co Done / removed Line 43: http://gerrit.ovirt.org/#/c/24594/40/frontend/webadmin/modules/webadmin/pom.xml File frontend/webadmin/modules/webadmin/pom.xml: Line 14: <!-- Google Web Toolkit dependencies --> Line 15: <dependency> Line 16: <groupId>org.gwtbootstrap3</groupId> Line 17: <artifactId>gwtbootstrap3</artifactId> Line 18: <version>0.5</version> > Please see my comment in gwt-common/pom.xml - I suggest to remove <version> Done Line 19: <scope>provided</scope> Line 20: </dependency> Line 21: <dependency> Line 22: <groupId>com.google.gwt</groupId> http://gerrit.ovirt.org/#/c/24594/40/frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/webadmin/WebAdmin.gwt.xml File frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/webadmin/WebAdmin.gwt.xml: Line 38: <extend-property name="locale" values="en_US,es_ES,fr_FR,ja_JP,pt_BR,zh_CN,de_DE,ko_KR" /> Line 39: <set-property name="locale" value="${gwt.locale}" /> Line 40: <set-property-fallback name="locale" value="en_US" /> Line 41: Line 42: <set-configuration-property name="CssResource.style" value="pretty" /> > UserPortal module inherits from GwtCommon module, so if GwtCommon module co Done / removed Line 43: -- To view, visit http://gerrit.ovirt.org/24594 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I806f2d163d80edb632c4e87524c327066cf3377a Gerrit-PatchSet: 40 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Greg Sheremeta <gsher...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com> Gerrit-Reviewer: Itamar Heim <ih...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@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
