Tomas Jelinek has posted comments on this change.
Change subject: webadmin: Introducing AddRemoveRowWidget
......................................................................
Patch Set 1:
(4 comments)
I miss the handling of the element IDs which can be used for selenium testing.
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/AddRemoveRowWidget.java
Line 84: addEntry(createGhostValue());
Line 85: }
Line 86:
Line 87: private void addEntry(T value) {
Line 88: final HorizontalPanel entry = new HorizontalPanel();
Please try to avoid using Horizontal/Vertical panels if it is at least a bit
possible. They are rendered as tables and unless it is really hard to style the
content using divs (FlowPanels) than we try to avoid using them.
Line 89: entry.addStyleName(style.entryStyle());
Line 90: contentPanel.add(entry);
Line 91:
Line 92: final V widget = createWidget(value);
Line 96:
Line 97: PushButton button = createButton(item);
Line 98: entry.add(button);
Line 99:
Line 100: final boolean ghost = isGhost(value);
can't you just call the isGhost() inside the valueChangeHandler so get rid of
this variable?
Line 101: toggleGhost(value, widget, ghost);
Line 102: widget.addValueChangeHandler(new ValueChangeHandler<T>() {
Line 103:
Line 104: private boolean wasGhost = ghost;
Line 107: public void onValueChange(ValueChangeEvent<T> event) {
Line 108: T value = event.getValue();
Line 109: boolean becomingGhost = isGhost(value);
Line 110: if (becomingGhost != wasGhost) {
Line 111: ((PushButton)
entry.getWidget(entry.getWidgetCount() - 1)).setEnabled(!becomingGhost);
this "entry.getWidgetCount() - 1" is not really readable and error prone... I
can see 3 ways how this could be done better:
1 (simple, not much better): rely on the fact that you know on which position
is the button and make a well named constant with that value (something like
BUTTON_POSITION = 1) - at least a bit readable
2 (more correct): mark the button you are looking for somehow (e.g.
getElement().setId() with a well known prefix or make a subclass of the
button) and look for that in the entry.
3: (maybe the best): create a custom child of the (in this case
HorizontalPanel) with a methods for getting the button and the editor.
Line 112: toggleGhost(value, widget, becomingGhost);
Line 113: wasGhost = becomingGhost;
Line 114: }
Line 115: }
Line 134: boolean ghostItem = isGhost(item.getFirst());
Line 135: final PushButton button =
Line 136: new PushButton(new Image(ghostItem ?
resources.increaseIcon() : resources.decreaseIcon()));
Line 137: button.addStyleName(style.buttonStyle());
Line 138: button.setEnabled(!ghostItem);
why disable for non-ghost?
Line 139:
Line 140: button.addClickHandler(ghostItem ?
Line 141: new ClickHandler() {
Line 142:
--
To view, visit http://gerrit.ovirt.org/19530
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I15958e4b7c1fcf0ec08ea35eb32c690964f2a366
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Lior Vernia <[email protected]>
Gerrit-Reviewer: Lior Vernia <[email protected]>
Gerrit-Reviewer: Tomas Jelinek <[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
