-
Notifications
You must be signed in to change notification settings - Fork 86
fix(deployments): show reasonable graph label defaults #2729
Conversation
Edit: Disregard this message for this PR, I found a neat issue that I had originally thought was associated with these changes, but can recreate it in master so that's fun. FWIW I've logged the issue here: openshiftio/openshift.io#3110 |
This is slightly unrelated, but while we're in this area: The networks section displays something like:
I think the second byte/s is redundant? This really could be another issue/PR though... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay to me.
netVal: number; | ||
netUnits: string; | ||
netUnits = 'bytes'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if it's obvious, I'd prefer to keep the type declarations on these properties, so netUnits: string = 'bytes';
@@ -258,8 +258,8 @@ export class DeploymentDetailsComponent { | |||
this.deploymentsService.getDeploymentNetworkStat(this.spaceId, this.environment, this.applicationId).subscribe((stats: NetworkStat[]) => { | |||
const last: NetworkStat = stats[stats.length - 1]; | |||
const netTotal: ScaledNetworkStat = new ScaledNetworkStat(last.received.raw + last.sent.raw); | |||
const decimals = netTotal.units === 'bytes' ? 0 : 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a type decl
@@ -17,7 +17,7 @@ | |||
<pfng-chart-sparkline class="chart-sparkline" [config]="cpuConfig" [chartData]="cpuData"></pfng-chart-sparkline> | |||
</div> | |||
<div class="chart-column-right"> | |||
<deployment-graph-label [ngClass]="cpuLabelClass" type="CPU" dataMeasure="Cores" [value]="cpuVal" [valueUpperBound]="cpuMax"></deployment-graph-label> | |||
<deployment-graph-label type="CPU" dataMeasure="Cores" [ngClass]="cpuLabelClass" [value]="cpuVal" [valueUpperBound]="cpuMax"></deployment-graph-label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some style guide reasoning behind reordering the attributes like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please retain/add type declarations
@@ -26,7 +26,7 @@ | |||
<pfng-chart-sparkline class="chart-sparkline" [config]="memConfig" [chartData]="memData"></pfng-chart-sparkline> | |||
</div> | |||
<div class="chart-column-right"> | |||
<deployment-graph-label [ngClass]="memLabelClass" type="Memory" [dataMeasure]="memUnits" [value]="memVal" [valueUpperBound]="memMax"></deployment-graph-label> | |||
<deployment-graph-label [dataMeasure]="memUnits" type="Memory" [ngClass]="memLabelClass" [value]="memVal" [valueUpperBound]="memMax"></deployment-graph-label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here as I had for the other file - is there some style guide reasoning for changing the order of the attributes?
|
||
describe('DeploymentGraphLabel', () => { | ||
const emptyChanges: SimpleChanges = {}; | ||
let component = new DeploymentGraphLabelComponent(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like component
can be const
, and could use a type declaration.
I think Koehler will be away for some time. I will take this over. I'll try to retain authorship, it shouldn't be too hard. |
fixes openshiftio/openshift.io/issues/2714