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

Enhancement: Progress Artifact #498

Merged
merged 24 commits into from
May 22, 2024
Merged

Conversation

dylanbhughes
Copy link
Contributor

@dylanbhughes dylanbhughes commented May 17, 2024

This PR adds handling for displaying dynamic progress artifacts on the flow run graph.

Screenshot 2024-05-22 at 5 07 18 PM

Copy link

netlify bot commented May 17, 2024

Deploy Preview for prefect-graphs ready!

Name Link
🔨 Latest commit 1b0974d
🔍 Latest deploy log https://app.netlify.com/sites/prefect-graphs/deploys/664e7271285b53000888442e
😎 Deploy Preview https://deploy-preview-498--prefect-graphs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@dylanbhughes dylanbhughes marked this pull request as ready for review May 22, 2024 21:08
@dylanbhughes dylanbhughes requested a review from a team as a code owner May 22, 2024 21:08
@dylanbhughes dylanbhughes changed the title Enhancement/progress artifact Enhancement: Progress Artifact May 22, 2024
Copy link
Collaborator

@pleek91 pleek91 left a comment

Choose a reason for hiding this comment

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

Overall LGTM! Few suggestions but nothing blocking.

Comment on lines +16 to +17
type: ArtifactType,
} & RunGraphArtifactTypeAndData
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since type exists on RunGraphArtifactTypeAndData It be little more readable and simpler to do this

{
  selected?: boolean,
  name?: string,
  artifact: RunGraphArtifactTypeAndData
}

Then you can just do options.artifact.type everywhere.

src/factories/artifactNode.ts Show resolved Hide resolved
src/factories/node.ts Outdated Show resolved Hide resolved
@dylanbhughes dylanbhughes merged commit 30530c0 into main May 22, 2024
4 checks passed
@dylanbhughes dylanbhughes deleted the enhancement/progress-artifact branch May 22, 2024 22:35
Copy link
Collaborator

@pleek91 pleek91 left a comment

Choose a reason for hiding this comment

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

Realize this is already merged. No rush to fix this. Prob fine to ignore but not totally sure. Otherwise we'll end up triggering renders because artifacts changed even if artifacts are disabled.

Comment on lines +194 to +195
artifactCacheKey && settings.disableArtifacts,
artifactCacheKey,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably not hurting anything but I think this is what we want here.

Suggested change
artifactCacheKey && settings.disableArtifacts,
artifactCacheKey,
artifactCacheKey && settings.disableArtifacts,

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.

None yet

3 participants