Vojtech Szocs has posted comments on this change. Change subject: userportal, webadmin: new look and feel based on PatternFly ......................................................................
Patch Set 40: (20 comments) Impressive patch, Greg! Nice work on integrating Bootstrap/gwtbootstrap3 into our GWT UI code base. Looking at changes, it sure wasn't an easy task. I have some comments (inline) but nothing major. Let me know what you think. 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? (This is not a splash page, code in pageLoaded function will have no effect here.) 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 example: <!-- Google Web Toolkit dependencies --> ... gwt-user + gwt-dev ... <!-- GWT wrapper for Bootstrap framework --> ... gwtbootstrap3 ... 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? [1] https://github.com/gwtbootstrap3/gwtbootstrap3/wiki/Initial-Setup Also, please remove <version> here (and in userportal-gwtp & webadmin pom.xml) and define it within <dependencyManagement> in frontend/webadmin/modules/pom.xml: ... <properties> ... <gwtbootstrap3.version>0.5</gwtbootstrap3.version> ... </properties> <dependencyManagement> ... <dependencies> <dependency> <groupId>org.gwtbootstrap3</groupId> <artifactId>gwtbootstrap3</artifactId> <version>${gwtbootstrap3.version}</version> </dependency> </dependencies> ... </dependencyManagement> ... 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 After reading gwtbootstrap3 wiki, I feel like using "GwtBootstrap3NoTheme" is the only viable option for existing GWT applications that already use some default GWT theme (and contain custom CSS to override it as necessary). Also, following line from their wiki sounds a bit scary: Tip: GwtBootstrap3's styles and widgets, especially the fluid grid system, work best when using the "classic" panels throughout your application and not the absolute positioned layout panels introduced in GWT 2.0. Since Bootstrap's grid system should be used for layout purposes, and since we use GWT layout panels (not classic panels) for layout purposes, we can potentially face some conflicts. However, at least for now, I'd still favor the use of GWT layout panels because of consistency/compliance with existing code, plus some widgets depend on resize callback (ProvidesResize interface) that is supported by GWT layout panels. 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 implementation. Just out of curiosity, does the default value "obf" break any integration with gwtbootstrap3? If not, we could use a Maven property to control CSS class name obfuscation, for example: <set-configuration-property name="CssResource.style" value="${gwt.cssResourceStyle}" /> (assuming relevant pom.xml would contain "cssResourceStyle" property, which could be "obf" by default) 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 This code is needed because here we no longer extend AbstractPopupPresenterWidget. AbstractPopupPresenterWidget's View component typically uses DialogBoxWithKeyHandlers which allows previewing (catching) all native JS events. AbstractPopupPresenterWidget essentially registers NativePreviewEvent handler that delegates to its onKeyPress method, which handles both ENTER and ESCAPE keys. 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/utils/PatternflyConstants.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/utils/PatternflyConstants.java: Line 1: package org.ovirt.engine.ui.common.utils; Line 2: Line 3: public class PatternflyConstants { Hm, I thought these could be provided by some gwtbootstrap3 constants class, I guess that's not the case though. Line 4: public static final String FORM_CONTROL = "form-control"; //$NON-NLS-1$ Line 5: public static final String FORM_GROUP = "form-group"; //$NON-NLS-1$ http://gerrit.ovirt.org/#/c/24594/40/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/AbstractUiCommandButton.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/AbstractUiCommandButton.java: Line 100: setLabel(command.getTitle()); Line 101: } Line 102: } Line 103: Line 104: protected abstract Widget getButtonWidget(); I see that you've changed return type to be generic Widget, but this way we'll have to do explicit type checks like you did above: Widget widget = getButtonWidget(); if (widget instanceof HasText) { // instanceof type check // do something with widget } Don't we generally want button widgets to implement some interfaces like HasText, HasClickHandlers, etc? If so, we can simply create some logical button interface like this: public interface ButtonDef extends HasText, HasClickHandlers ... and modify getButtonWidget method to work with ButtonDef instance. Line 105: 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. 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. 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. 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. 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. 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: return nextTabIndex; (Returning 0 will break tab index sequencing and yield undesired behavior in browser.) 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> here, relying on <dependencyManagement> of parent POM instead. 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 9: import com.google.gwt.user.client.ui.HTMLPanel; Line 10: import com.google.gwt.user.client.ui.Widget; Line 11: import com.gwtplatform.mvp.client.TabData; Line 12: Line 13: public class UserPortalMainTab implements TabDefinition { Hm, I assume that implementing TabDefinition interface directly here is because of AbstractTab's UX design assumptions, i.e. requiring implementations to provide UiBinder template with leftElement, rightElement, middleElement etc. However, by implementing TabDefinition interface directly, we might be duplicating code already present in AbstractTab. What we could do: * new AbstractTab would contain only common bits between this class and current AbstractTab * current AbstractTab would be renamed to CompositeAbstractTab (indicating that its implementation is composition of left/right/middle/etc. elements) and would extend new AbstractTab * this class would simply extend new AbstractTab Line 14: Line 15: private float priority; Line 16: private boolean accessible; Line 17: private AbstractTabPanel tabPanel; 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. 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 contains override of GWT config property, this override is carried over into this module. (In other words, I don't think we need this override here, it's already present in GwtCommon module.) 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> here, relying on <dependencyManagement> of parent POM instead. 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 contains override of GWT config property, this override is carried over into this module. (In other words, I don't think we need this override here, it's already present in GwtCommon module.) 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
