Skip to content

[1/n][Asset Health] Add descriptions to health summary card #29256

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

Merged
merged 1 commit into from
Apr 14, 2025

Conversation

salazarm
Copy link
Contributor

@salazarm salazarm commented Apr 14, 2025

Summary & Motivation

  1. Add descriptions.
  2. Fix the order of events in the health trend.

How I Tested These Changes

Screenshot 2025-04-14 at 12 09 29 PM Screenshot 2025-04-14 at 12 09 09 PM Screenshot 2025-04-14 at 12 09 02 PM Screenshot 2025-04-14 at 12 08 57 PM Screenshot 2025-04-14 at 12 08 50 PM Screenshot 2025-04-14 at 12 22 24 PM Screenshot 2025-04-14 at 12 22 17 PM

@salazarm salazarm changed the title [Asset Health] Add descriptions to health summary card [1/n][Asset Health] Add descriptions to health summary card Apr 14, 2025
Copy link

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-bxre1xpim-elementl.vercel.app
https://salazarm-observe-ui-helper-text.core-storybook.dagster-docs.io

Built with commit ee16d57.
This pull request is being automatically deployed with vercel-action

<Body>
{numberFormatter.format(metadata.numWarningChecks)}/
{numberFormatter.format(metadata.totalNumChecks)}{' '}
<Link to={assetDetailsPathForKey(assetKey, {view: 'checks'})}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

3 checks warning feels a bit odd but I guess technically correct!

Comment on lines +175 to +180
{numberFormatter.format(metadata.numWarningChecks)}/
{numberFormatter.format(metadata.totalNumChecks)}{' '}
<Link to={assetDetailsPathForKey(assetKey, {view: 'checks'})}>
check
{ifPlural(metadata.totalNumChecks, '', 's')}
</Link>{' '}
Copy link
Collaborator

Choose a reason for hiding this comment

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

feels vaguely like a checkStatus(metadata.numWarningChecks, metadata.totalNumChecks, 'warning') style helper could make this more readable, hard to see what is changing between all these cases

Probably not worth changing though

...AssetHealthCheckDegradedMetaFragment
...AssetHealthCheckWarningMetaFragment
...AssetHealthCheckUnknownMetaFragment
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this stuff heavy? Was sort of imagining we'd fetch this on-hover but maybe it's not that big of a lift to pull the partition counts :-o 🎉

@salazarm salazarm merged commit 424b17a into master Apr 14, 2025
7 checks passed
@salazarm salazarm deleted the salazarm/observe-ui-helper-text branch April 14, 2025 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants