-
Notifications
You must be signed in to change notification settings - Fork 231
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
Add build failure reasons to monitoring and overview pages #996
Add build failure reasons to monitoring and overview pages #996
Conversation
@spadgett missed this two pages, i couldn't decide between the long or short form on the overview |
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.
Changes LGTM, just one question on the build-status
directive.
@@ -303,7 +303,9 @@ | |||
<small>created <span am-time-ago="build.metadata.creationTimestamp"></span></small> | |||
</div> | |||
<div class="list-group-item-text"> | |||
<build-status build="build"></build-status> |
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.
Should we delete the build-status directive if it's not used? Or update it to show the reason or message instead?
Yeah I think we need to use "1m 48s" format, maybe small / text-muted? |
i dunno about changing the font style, in some cases its read as part of a statement like with Cancelled |
9466db7
to
aba973e
Compare
updated PTAL |
[merge] |
Evaluated for origin web console merge up to aba973e |
Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/820/) (Base Commit: 8fd9c66) |
No description provided.