New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1916834: Pipeline metrics text updates #7850
Bug 1916834: Pipeline metrics text updates #7850
Conversation
/kind bug |
@rottencandy: This pull request references Bugzilla bug 1916834, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cc @bgliwa01 |
y: parseFloat(percentage), | ||
name: `${obj.x}: ${percentage}%`, | ||
}; | ||
}); | ||
let successTimeSeriesObj = finalArray.reduce((acc, obj) => { | ||
if (obj.x !== 'success') return acc; | ||
if (obj.x !== 'Success') return acc; |
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.
Shouldn't we use an enum for these checks?? something like Status.Success
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.
I was not able to find an existing enum for pipeline graph axes. Should I create one?
Not sure if it's required since we only check for the Success
axis here.
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.
not very necessary, but it would be really good, if you can store this string const successString = 'Success'
in a variable as it is being checked in multiple places, IMO
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.
Updated.
c177444
to
6b43579
Compare
@@ -36,6 +36,7 @@ const PipelineSuccessRatioDonut: React.FC<PipelineMetricsGraphProps> = ({ | |||
delay: interval, | |||
}); | |||
const pipelineSuccessData = runData?.data?.result ?? []; | |||
const successString = 'Success'; |
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.
Any reason to capitalize this string? I don't see it being rendered anywhere. You wouldn't have to explicitly capitalize strings in the obj to check for it's equality if you just kept it success
.
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.
It is used by the donut chart component on hover. I realized It could be capitalized directly inside the component itself. Updated.
6b43579
to
14c9cb7
Compare
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rohitkrai03, rottencandy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
In the first image, what is the black box that says pipeline success ratio? |
@rottencandy: All pull requests linked via external trackers have merged: Bugzilla bug 1916834 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Fixes:
https://issues.redhat.com/browse/ODC-5310
Analysis / Root cause:
Solution Description:
Update the text
Screen shots / Gifs for design review:
cc: @openshift/team-devconsole-ux
Browser conformance: