Alexander Wels has posted comments on this change. Change subject: webadmin : CheckBoxGroup, DaysOfMonthSelector and DateTimeBox Widgets ......................................................................
Patch Set 11: (8 comments) Overall its significantly improved (I hope you agree). Just a bunch of little nitpicky stuff left, but much better. 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 duplicate key, you don't have check here again. 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 145: toArray You might want to check if there is at least (and at most as well) one value in the collection, this will throw an exception if the values collection is empty. 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. 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 you clicked on it. 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 selectedFlexTableStyle here. Line 65: } Line 66: }); Line 67: } Line 68: Line 88: addClassName You have gone through all the trouble to get the style defined here. so you can now do addClassName(style.normalFlexTableCell()); 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 different version, GWT bootstrap and GWT bootstrap 3). Make sure you are using the GWT bootstrap 3 version. 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 a widget like that, that is really not global, put the css in its own .css file like you did with the checkbox. 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 <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Einav Cohen <[email protected]> Gerrit-Reviewer: Greg Sheremeta <[email protected]> Gerrit-Reviewer: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[email protected]> Gerrit-Reviewer: anmolbabu <[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
