On 6.6.2014 15:45, Endi Sukma Dewata wrote:
On 6/5/2014 9:25 AM, Endi Sukma Dewata wrote:
ACK for patches #592-#628. I'll continue reviewing the rest.
ACK for patches #633-639, #642, #644, #652, and #653. Patches #640 &
#641 have an issue (see #19 below) that should be fixed before pushing.
Other issues are minor/unrelated/suggestions that can be addressed
separately.
Thank you for the review.
I've fixed issues:
- #19, #20 in patch #640.
And some low hanging fruit:
- #16, in patch #637
- #17, in patch #612
- #13, in patch #612
The branch has been rebased to current master.
Some comments bellow. For the rest (including issues #1-#12) I'll create
tickets and/or fix them later. Some are existing issues.
I am not able to reproduce issues #4 and #11.
13. The separators in the top right drop down menu shouldn't be
focusable. To test this, click the menu, then hit tab several times.
Fixed. Also disabled items are no longer focusable.
14. In the certificates page, if I enter a search filter then hit
Refresh (instead of Enter) it doesn't change the content based on the
new search filter. I suppose a more intuitive behavior would be to rerun
the search with the new filter, or reset the filter to the previous value.
15. In the certificates page, if I enter an invalid filter it will show
an error dialog, then after I close it it will show the error message in
the page. This is fine, but if I go to another page, then back, it will
do the same thing as if the search is executed again. If the page is
cached, it probably shouldn't display the error dialog, just the error
message in the page. Alternatively, if the search failed it shouldn't be
cached.
I don't think this page is cached -> the wrong command is executed every
time which will show the dialog.
16. In the association adder dialog it's not clear if the filter applies
to just the Available list or the Prospective list too. Maybe the
placeholder text could say "Filter available ${other_entity}".
Fixed
17. It looks like some facet-actions elements contain unnecessary blank
ul.dropdown-menu elements which probably correspond to the number of
menu items (see User's Actions button).
Fixed DropdownWidget's _render_items method.
18. In the New Certificate dialog for Host, the instruction to create a
CSR exceeds the dialog boundary.
caused by BS' CSS:
code {
white-space: nowrap;
}
I wonder if the best solution is to reset it to initial value in all
dialogs.
19. The "View certificate" is missing from the Host's and Service's
Action, probably because of the incorrect labels below:
IPA.cert.view_action = function(spec) {
// should be objects.cert.view_certificate_btn
spec.label = spec.label || '@i18n:objects.cert.view';
that.execute_action = function(facet) {
// should be objects.cert.view_certificate
var title = text.get('@i18n:objects.cert.view_certificate_btn');
Fixed
20. The capitalization of "Certificate" is inconsistent in Host's and
Service's Actions.
Fixed
21. Should there be a link from the Host page to the Certificate page?
Can the certificate serial number be displayed in the Host page? If we
do this, it's probably not necessary to have Revoke/Restore Certificate
actions in the Host page. Same thing with the Service page.
22. The add dialog for RADIUS Server uses a field label "Secret".
Everywhere else in the RADIUS Server page it's called "Password" (e.g.
Verify Password, Reset Password).
--
Petr Vobornik
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
