Vojtech Szocs has posted comments on this change. Change subject: userportal, webadmin: new tooltip infrastructure ......................................................................
Patch Set 17: (7 comments) Very nice patch! I really appreciate the helpful Javadocs and TODO-GWT markers. Posted some small comments/questions, but it's basically +2 from me. https://gerrit.ovirt.org/#/c/36597/17/backend/manager/modules/branding/src/main/resources/META-INF/tags/obrand/javascripts.tag File backend/manager/modules/branding/src/main/resources/META-INF/tags/obrand/javascripts.tag: Line 3: <%@ taglib prefix="c" uri="http://java.sun.com/jsp/jstl/core"; %> Line 4: <%@ taglib prefix="fn" uri="http://java.sun.com/jsp/jstl/functions"; %> Line 5: Line 6: <c:set var="baseTheme" value="${requestScope['brandingStyle'][0]}" /> Line 7: <script type="text/javascript" src="${pageContext.request.contextPath}${initParam['obrandThemePath']}${baseTheme.path}/patternfly/components/jquery/jquery.min.js"></script> In our GWT apps, do we inherit "Bootstrap" or "BootstrapNoResources" module? * https://github.com/gwtbootstrap/gwt-bootstrap/blob/master/src/main/java/com/github/gwtbootstrap/Bootstrap.gwt.xml * https://github.com/gwtbootstrap/gwt-bootstrap/blob/master/src/main/java/com/github/gwtbootstrap/BootstrapNoResources.gwt.xml If we inherit "Bootstrap" module (with resources), AFAIK, jQuery (along with other stuff) is already bundled and injected upon GWT application startup. * https://github.com/gwtbootstrap/gwt-bootstrap/tree/master/src/main/java/com/github/gwtbootstrap/client/ui/resources/js * https://github.com/gwtbootstrap/gwt-bootstrap/tree/master/src/main/java/com/github/gwtbootstrap/client/ui/resources/css * etc. If so, I think we don't need jQuery <script> tag above. Please let me know if I'm missing something. Line 8: <script type="text/javascript" src="${pageContext.request.contextPath}${initParam['obrandThemePath']}${baseTheme.path}/mousetrack.js"></script> Line 9: <c:choose> Line 10: <c:when test="${fn:containsIgnoreCase(header['User-Agent'],'MSIE 8.0')}"> Line 11: <script type="text/javascript" src="${pageContext.request.contextPath}${initParam['obrandThemePath']}${baseTheme.path}/patternfly/components/respond/dest/respond.min.js"></script> https://gerrit.ovirt.org/#/c/36597/17/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/AbstractTooltipCell.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/AbstractTooltipCell.java: Line 54: * Line 55: * @see com.google.gwt.cell.client.AbstractCell#onBrowserEvent(com.google.gwt.cell.client.Cell.Context, com.google.gwt.dom.client.Element, java.lang.Object, com.google.gwt.dom.client.NativeEvent, com.google.gwt.cell.client.ValueUpdater) Line 56: */ Line 57: @Override Line 58: public void onBrowserEvent(Context context, Element parent, C value, NativeEvent event, ValueUpdater<C> valueUpdater) { If this method isn't expected to be overridden any further, consider making it final. Line 59: onBrowserEvent(context, parent, value, null, event, valueUpdater); Line 60: } Line 61: Line 62: /** Line 98: * Override the normal render to pass along an id. Line 99: * Line 100: * @see com.google.gwt.cell.client.AbstractCell#render(com.google.gwt.cell.client.Cell.Context, java.lang.Object, com.google.gwt.safehtml.shared.SafeHtmlBuilder) Line 101: */ Line 102: public void render(Context context, C value, SafeHtmlBuilder sb) { If this method isn't expected to be overridden any further, consider making it final. Line 103: String id = ElementIdUtils.createTableCellElementId(getElementIdPrefix(), getColumnId(), context); Line 104: render(context, value, sb, id); Line 105: } Line 106: https://gerrit.ovirt.org/#/c/36597/17/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/tooltip/ElementTooltip.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/tooltip/ElementTooltip.java: Line 293: * mouse away -- but for some reason the mouseout doesn't fire. In this case, a tooltip will render Line 294: * over an Element the mouse is no longer over. So we check for this condition, and simply cancel Line 295: * the render if it is true. Line 296: * Line 297: * If any issues with hanging tooltips creep up, start debugging here :) :-) Line 298: * Line 299: * @param event Event Line 300: */ Line 301: protected void onShow(final Event event) { https://gerrit.ovirt.org/#/c/36597/17/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/tooltip/Tooltip.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/tooltip/Tooltip.java: Line 50: * ======================================================================================== Line 51: * oVirt customization of gwtbootstrap3 tooltip Line 52: * ======================================================================================== Line 53: * TODO-GWT switch back to using native gwtbootstrap3 tooltip when upstream patch Line 54: * xxx is merged and released. Which patch does "xxx" refer to? :-) Line 55: * ======================================================================================== Line 56: * ======================================================================================== Line 57: * Line 58: * Basic implementation for the Bootstrap tooltip https://gerrit.ovirt.org/#/c/36597/17/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/MainTabClusterView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/MainTabClusterView.java: Line 60: CommentColumn2<VDSGroup> commentColumn = new CommentColumn2<VDSGroup>(); Line 61: // TODO: add support for tooltips on headers Line 62: // TODO: don't hardcode "Comment" -- use image Line 63: // getTable().addColumnWithHtmlHeader(commentColumn, commentColumn.getHeaderHtml(), "30px"); //$NON-NLS-1$ Line 64: getTable().addColumn(commentColumn, "Comment", "50px"); //$NON-NLS-1$ //$NON-NLS-2$ I assume this change is for testing only and will be reverted before merge? Line 65: Line 66: if (ApplicationModeHelper.getUiMode() != ApplicationMode.GlusterOnly) { Line 67: AbstractTextColumnWithTooltip<VDSGroup> dataCenterColumn = new AbstractTextColumnWithTooltip<VDSGroup>() { Line 68: @Override https://gerrit.ovirt.org/#/c/36597/17/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/table/column/CommentColumn2.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/table/column/CommentColumn2.java: Line 16: * actual comment in a tooltip. Line 17: * Line 18: * @param <T> row type Line 19: */ Line 20: public class CommentColumn2<T extends Commented> extends AbstractColumn<T, ImageResource> { Is this class meant to replace existing CommentColumn? Line 21: Line 22: private static final CommonApplicationResources RESOURCES = GWT.create(CommonApplicationResources.class); Line 23: Line 24: public CommentColumn2() { -- To view, visit https://gerrit.ovirt.org/36597 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie60ebe16e2d8830498fd819ffdb39aa043daadd2 Gerrit-PatchSet: 17 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Greg Sheremeta <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Einav Cohen <[email protected]> Gerrit-Reviewer: Greg Sheremeta <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[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
