Alexander Wels has posted comments on this change.

Change subject: webadmin: Host SSH Fingerprint success message shouldn't be in 
red text
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.ovirt.org/#/c/33729/1/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/HostPopupView.java
File 
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/HostPopupView.java:

Line 697:             @Override
Line 698:             public void eventRaised(Event ev, Object sender, 
EventArgs args) {
Line 699:                 String fetchResultText = 
object.getFetchResult().getEntity();
Line 700:                 if 
(ConstantsManager.getInstance().getConstants().errorLoadingFingerprint().equals(fetchResultText))
 {
Line 701:                     
fetchResult.setStyleName(style.fetchResultErrorLabel());
I would do:

  fetchResult.addStyleName(style.fetchResultErrorLabel());

and

  fetchResult.removeStyleName(style.fetchResultErrorLabel());

in the case it is successful. The remove won't do anything if the style is not 
there.
Line 702:                 } else {
Line 703:                     
fetchResult.setStyleName(style.fetchResultLabel());
Line 704:                 }
Line 705:                 fetchResult.setText(fetchResultText);


Line 1093:         String expanderContent();
Line 1094: 
Line 1095:         String pkStyle();
Line 1096: 
Line 1097:         String fetchResultLabel();
If you take my suggestion above you don't need this.
Line 1098: 
Line 1099:         String fetchResultErrorLabel();
Line 1100:     }
Line 1101: 


http://gerrit.ovirt.org/#/c/33729/1/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/HostPopupView.ui.xml
File 
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/HostPopupView.ui.xml:

Line 60:             margin-left: 4px;
Line 61:         }
Line 62: 
Line 63:         .fetchResultErrorLabel {
Line 64:             font-color: red;
font-color is not an actual css property, so can be removed.
I would put all the common/default attributes in the fetchResultLabel like so:

  .fetchResultLabel {
    color: black;
    margin-left: 4px;
  }

Then put the attributes that change in the error class like so:

  .fetchResultErrorLabel {
    color: red;
  }

Then add/remove the error label based on the result as demonstrated by my 
example in HostPopupView.java. This allows you not have to alter 2 classes when 
some base/default property changes (like margin-left.
Line 65:             color: red;
Line 66:             margin-left: 4px;
Line 67:         }
Line 68: 


-- 
To view, visit http://gerrit.ovirt.org/33729
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I28c5a3c82202733a9a628b80a50634c7ccb19d85
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ravi Nori <rn...@redhat.com>
Gerrit-Reviewer: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com>
Gerrit-Reviewer: Ravi Nori <rn...@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

Reply via email to