Bug 1796641: Add error state to Notification Drawer #4130
Conversation
openshift-ci-robot
added
bugzilla/invalid-bug
|
/bugzilla refresh |
openshift-ci-robot
added
bugzilla/valid-bug
|
@jcaianirh: This pull request references Bugzilla bug 1796641, 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. 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. |
|
@jcaianirh: This pull request references Bugzilla bug 1796641, which is valid. 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. |
|
/test analyze |
| </Link> | ||
| </EmptyStateSecondaryActions> | ||
| <EmptyStateIcon className="co-status-card__alerts-icon" icon={UnknownIcon} /> | ||
| <EmptyStateBody>Alerts could not be loaded.</EmptyStateBody> |
There was a problem hiding this comment.
I'd suggest showing the error details with the message here.
There was a problem hiding this comment.
It will be hard to troubleshoot what's wrong if we don't show any error details.
The dashboard page is maybe a little different because we want to avoid showing the same error again and again on every card if prometheus is down. @cshinn ?
There was a problem hiding this comment.
I was unaware that we would get any more information other than "not responding". If we have a more useful message to show, I think it makes sense to do so
There was a problem hiding this comment.
cool, @cshinn do you have an idea for where to display the actual error?
|
@spadgett moved error/empty states inside component. |
|
Thanks @jcaianirh, this looks a lot cleaner 👍 Let me know when you have the error message for another review. |
|
/retest |
@jcaianirh This looks good and it matches well with how we display these messages elsewhere in the console. The only change I would make is to reduce the bottom margin a bit so that it matches the margin to the sides of the notification box. Not a critical change though. |
|
@cshinn updated error detail alignment. thanks for the comment.
|
|
@jcaianirh The margins look off in that screenshot, maybe because we're only setting the body and not empty state title. Should it really be the unknown icon when there is an error? I'd expect an error icon. Or maybe no icon if the alert has the icon. @cshinn opinion? |
|
@spadgett That version with the <?> was meant to match the dashboard error state, but since we've got more information to show here a couple other options. This one is based on the empty state pattern This one is similar to the current implementation and what appears to be the error state of metrics dashboard cards |
|
Thanks @cshinn. My personal preference is for the first option. It's already clearly an error, so I don't think we need to duplicate the icon and color with the inline alert. |
|
/approve Agree, prefer the first option! |
openshift-ci-robot
added
the
approved
|
/retest |
1 similar comment
|
/retest |
|
@spadgett updated the design as noted above. thanks
|
|
@spadgett and @benjaminapetersen this is set for final review |
|
/retest |
|
@spadgett squashed. thanks |
|
/test analyze |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benjaminapetersen, jcaianirh, spadgett 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. |
4 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@jcaianirh: All pull requests linked via external trackers have merged. Bugzilla bug 1796641 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://bugzilla.redhat.com/show_bug.cgi?id=1796641