[Freeipa-devel] [PATCH] 592-628 Update to PatternFly

[Petr Vobornik]

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