anmolbabu has posted comments on this change. Change subject: webadmin : CheckBoxGroup, DaysOfMonthSelector and DateTimeBox Widgets ......................................................................
Patch Set 11: (8 comments) https://gerrit.ovirt.org/#/c/37302/11/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/CheckBoxGroup.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/CheckBoxGroup.java: Line 63: String checkBoxLabel = renderer.render(checkBoxValue); Line 64: if (checkBoxLabel == null) { Line 65: throw new IllegalArgumentException("null value is not permited"); //$NON-NLS-1$ Line 66: } Line 67: if (checkBoxes.containsKey(checkBoxValue)) { > Since the only two places that call this method already check if there is a Done Line 68: throw new IllegalArgumentException("Duplicate value: " + checkBoxLabel); //$NON-NLS-1$ Line 69: } Line 70: final CheckBox newCheckBox = buildCheckbox(checkBoxValue); Line 71: checkBoxes.put(checkBoxValue, newCheckBox); Line 141: } Line 142: Line 143: @Override Line 144: public void setAcceptableValues(Collection<List<T>> values) { Line 145: List<T> acceptableValues = (List<T>) values.toArray()[0]; > You might want to check if there is at least (and at most as well) one valu Done. Line 146: for(T currentValue : acceptableValues) { Line 147: if(!checkBoxes.containsKey(currentValue)) { Line 148: addCheckBox(currentValue); Line 149: } https://gerrit.ovirt.org/#/c/37302/11/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/DaysOfMonthSelector.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/DaysOfMonthSelector.java: Line 37: // Starts from index 0 and goes upto 31(Assumed to be last day of month(recurrence)) Line 38: List<Boolean> clickedList = new ArrayList<>(); Line 39: Line 40: public interface Resources extends ClientBundle { Line 41: @Source("org/ovirt/engine/ui/common/css/DaysOfMonthSelector.css.css") > I am pretty sure there only needs to be one .css here. haha that was a typo. thanks :) Line 42: DaysOfMonthSelectorCss daysOfMonthSelectorCSS(); Line 43: } Line 44: Line 45: public DaysOfMonthSelector() { Line 56: int cellRow = cellClicked.getRowIndex(); Line 57: int actualCellIndex = cellRow * DAYS_IN_WEEK + cellColumn + 1; Line 58: if (!clickedList.get(actualCellIndex)) { Line 59: clickedList.set(actualCellIndex, true); Line 60: onSelectedItemsChange(actualCellIndex, true); > You probably want to add the selectedFlexTableCell style here, to indicate Its done in onSelectedItemsChange Line 61: } else { Line 62: clickedList.set(actualCellIndex, false); Line 63: onSelectedItemsChange(actualCellIndex, false); Line 64: } Line 60: onSelectedItemsChange(actualCellIndex, true); Line 61: } else { Line 62: clickedList.set(actualCellIndex, false); Line 63: onSelectedItemsChange(actualCellIndex, false); Line 64: } > The reverse of above here, make sure it doesn't have the selectedFlexTableS Same as above Line 65: } Line 66: }); Line 67: } Line 68: Line 84: row++; Line 85: column = 0; Line 86: } Line 87: daysOfMonth.setWidget(row, column, new Label(Integer.toString(i + 1))); Line 88: daysOfMonth.getCellFormatter().getElement(row, column).addClassName("normalFlexTableCell");//$NON-NLS-1$ > You have gone through all the trouble to get the style defined here. so you Yes missed it here. Line 89: clickedList.add(i, false); Line 90: column++; Line 91: } Line 92: Label widget = new Label(constants.lastDay()); https://gerrit.ovirt.org/#/c/37302/11/frontend/webadmin/modules/webadmin/pom.xml File frontend/webadmin/modules/webadmin/pom.xml: Line 143: <version>${engine.version}</version> Line 144: <type>test-jar</type> Line 145: <scope>test</scope> Line 146: </dependency> Line 147: <dependency> > I know we already have the GWT bootstrap 3 code included (note there are 2 Done Line 148: <groupId>com.github.gwtbootstrap</groupId> Line 149: <artifactId>gwt-bootstrap</artifactId> Line 150: <version>2.0.4.0</version> Line 151: </dependency> https://gerrit.ovirt.org/#/c/37302/11/frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/webadmin/public/WebAdmin.css File frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/webadmin/public/WebAdmin.css: Line 114: /* right click menu spacing */ Line 115: .gwt-MenuBar .gwt-MenuItem { Line 116: padding: 0 10px !important; Line 117: } Line 118: > The same thing goes as what I said with the CheckBox css, if you are making Done Line 119: .daysSelectorWrapperPanel { Line 120: width: 190px; Line 121: } Line 122: -- To view, visit https://gerrit.ovirt.org/37302 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I38daa0d2c151eb0e34603488496a8a1ea4719c87 Gerrit-PatchSet: 11 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: anmolbabu <anb...@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: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: anmolbabu <anb...@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
