Skip to content
This repository has been archived by the owner on May 7, 2021. It is now read-only.

fix(deployments): show reasonable graph label defaults #2729

Closed
wants to merge 1 commit into from

Conversation

c-koehler
Copy link
Contributor

@aptmac
Copy link
Member

aptmac commented Apr 12, 2018

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

@jiekang
Copy link
Contributor

jiekang commented Apr 13, 2018

This is slightly unrelated, but while we're in this area:

The networks section displays something like:

Network (bytes/s)

697 bytes/s

I think the second byte/s is redundant? This really could be another issue/PR though...

Copy link
Contributor

@jiekang jiekang left a 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.

andrewazores
andrewazores previously approved these changes Apr 13, 2018
netVal: number;
netUnits: string;
netUnits = 'bytes';
Copy link
Member

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;
Copy link
Member

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>
Copy link
Member

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?

@andrewazores andrewazores dismissed their stale review April 13, 2018 16:11

wrong review status

Copy link
Member

@andrewazores andrewazores left a 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>
Copy link
Member

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();
Copy link
Member

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.

@jiekang
Copy link
Contributor

jiekang commented Apr 18, 2018

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[3] (Deployments) Show reasonable defaults for deployments statistics
4 participants