Skip to content
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

Adds orange/red warning/error icons for exited resources on resources dashboard page #1378

Merged
merged 15 commits into from
Dec 21, 2023

Conversation

adamint
Copy link
Member

@adamint adamint commented Dec 13, 2023

Fixes #533

Adds warning icon to exited resources with a zero or null exit code, and an error icon (and tooltip that mentions the exit code) to exited resources with a non-zero, non-null exit code.

The badge is clickable and takes you to console logs for that resource.

image

Microsoft Reviewers: Open in CodeFlow

@adamint adamint changed the title Dev/adamint/533 finished errored Adds orange/red warning/error icons for exited resources on resources dashboard page Dec 13, 2023
Copy link

@leslierichardson95 leslierichardson95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall! My one suggestion: can we change the icon in the orange example to either the same exclamation point as the red or to something else? I was a little confused about which icon represented the error and which represented the warning otherwise

@dotnet-policy-service dotnet-policy-service bot added the needs-author-action An issue or pull request that requires more info or actions from the author. label Dec 13, 2023
@tlmii
Copy link
Member

tlmii commented Dec 13, 2023

Out of curiosity, are we aware of any prior art of other applications showing the exit code from a process in the UI? I'm almost tempted to see what it'd look like to put the actual exit code in the badge (if it is an error; leave it as an icon if it is not).

Also, #533 suggested changing the background color when it is in a finished state as well. Did you think the icon was sufficient instead?

@JamesNK
Copy link
Member

JamesNK commented Dec 14, 2023

How come the icons are inside a badge instead of being a "filled" icon? That's consistent with the icons displayed in structured logs and console logs.

image

# Conflicts:
#	src/Aspire.Dashboard/Components/Pages/ConsoleLogs.razor.cs
#	src/Aspire.Dashboard/Components/Pages/Resources.razor
#	src/Aspire.Dashboard/Components/Pages/Resources.razor.cs
#	src/Aspire.Dashboard/Components/ResourcesGridColumns/UnreadLogErrorsBadge.razor
#	src/Aspire.Dashboard/Resources/Columns.Designer.cs
#	src/Aspire.Dashboard/Resources/xlf/Columns.cs.xlf
#	src/Aspire.Dashboard/Resources/xlf/Columns.de.xlf
#	src/Aspire.Dashboard/Resources/xlf/Columns.es.xlf
#	src/Aspire.Dashboard/Resources/xlf/Columns.fr.xlf
#	src/Aspire.Dashboard/Resources/xlf/Columns.it.xlf
#	src/Aspire.Dashboard/Resources/xlf/Columns.ja.xlf
#	src/Aspire.Dashboard/Resources/xlf/Columns.ko.xlf
#	src/Aspire.Dashboard/Resources/xlf/Columns.pl.xlf
#	src/Aspire.Dashboard/Resources/xlf/Columns.pt-BR.xlf
#	src/Aspire.Dashboard/Resources/xlf/Columns.ru.xlf
#	src/Aspire.Dashboard/Resources/xlf/Columns.tr.xlf
#	src/Aspire.Dashboard/Resources/xlf/Columns.zh-Hans.xlf
#	src/Aspire.Dashboard/Resources/xlf/Columns.zh-Hant.xlf
#	src/Aspire.Hosting/Dashboard/DcpDataSource.cs
@adamint
Copy link
Member Author

adamint commented Dec 19, 2023

How come the icons are inside a badge instead of being a "filled" icon? That's consistent with the icons displayed in structured logs and console logs.

We can do this, but realistically requires us to remove the ability to click - we have a fairly widespread problem with icons with clickable parent divs that are non-compliant (which I accidentally contributed to here, oops! see #1392), and the real solution here is a binary: if we want to make an icon clickable, it needs to be turned into a button, and if we want to use a filled icon, the icon cannot be clickable.

I'll convert to a filled icon and disallow clicking.

@JamesNK

@dotnet-policy-service dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Dec 19, 2023
@dotnet-policy-service dotnet-policy-service bot added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Dec 20, 2023
@RussKie
Copy link
Member

RussKie commented Dec 20, 2023

Please make sure the colours are colourblind-friendly. Also, please keep the accessibility in mind - some users rely on assistive technology (e.g., screen readers, etc.).

@adamint
Copy link
Member Author

adamint commented Dec 20, 2023

Please make sure the colours are colourblind-friendly.

This was not a requirement I was aware of. We use red pervasively throughout the dashboard to indicate error. However, the redness is not the only indication of the state, which I thought made us compliant. @RussKie

Also, please keep the accessibility in mind - some users rely on assistive technology (e.g., screen readers, etc.).

Yes, keyboard navigability and screen-reader support is one of the items I'm working on this sprint.

@adamint
Copy link
Member Author

adamint commented Dec 20, 2023

Ok, this is ready for another look. I put the warning and error icons before the state, and they have tooltips. They are not clickable.

I added an anchor to the badge, so it appears as a direct link.
image

@leslierichardson95 @tlmii @leslierichardson95

@adamint adamint requested a review from JamesNK December 21, 2023 18:32
@adamint adamint dismissed JamesNK’s stale review December 21, 2023 19:56

addressed feedback, approved by leslie

@adamint adamint merged commit 63497fd into dotnet:main Dec 21, 2023
8 checks passed
drewnoakes added a commit to drewnoakes/aspire that referenced this pull request Dec 22, 2023
Removes code made unused in dotnet#1378 that triggers build errors.
drewnoakes added a commit to drewnoakes/aspire that referenced this pull request Dec 22, 2023
Removes code made unused in dotnet#1378 that triggers build errors.
@drewnoakes drewnoakes mentioned this pull request Dec 22, 2023
drewnoakes added a commit that referenced this pull request Dec 22, 2023
Removes code made unused in #1378 that triggers build errors.
@tlmii
Copy link
Member

tlmii commented Jan 3, 2024

@kvenkatrajan I had a similar thought, so I went ahead and created a new issue from your comment: #1534

@tlmii tlmii mentioned this pull request Jan 3, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it easy to identify services are logging errors on the projects/executables pages
6 participants